Detection Handler Tests #722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mocked all remaining interfaces on Server and refactored utilhandler to use it as an extremely bare bones POC.
Added go:generate line to mock DetectionEngine. To prevent circular imports, handler functions need to be made public for tests. Added tests for GetDetection and GetByPublicID providing 100% coverage of their handlers. The CreateDetection test is in-progress. Added EntryMatcher as a way to test logs for particular levels, fields, and messages. The plan is the test creates a parallel array with conditions on each EntryMatcher that test their corresponding log entry. A matcher is stateless and can be reused. Added a few helper functions.
While writing tests for CreateDetection, it was discovered that we call ApplyFilters twice and this was causing a logic error because we only tracked if the status changed the second time but the first time is when the change will take place. This prevented CreateDetection from ever indicating that the created detection was modified. Built a better way to test responses with a type switch and updated existing tests to use it. TODO: Evaluate if UpdateDetection should call ExtractDetails before PrepareToSave (which calls ExtractDetails again).
Calling ExtractDetails directly before PrepareForSave is unnecessary as the first thing PrepareForSave does is call ExtractDetails. UpdateDetection cannot return some of the errors that it checked for. Those errors were caught by PrepareForSave. Simplified the list of errors checked. When a Detection is disabled because it failed to sync while enabled, we failed to call MergeAuxData. Refactored to fix this. More tests for UpdateDetection.
While writing tests for DeleteDetection, I noticed the store wasn't checking permissions, this means a user without write/detections permissions could delete a detection and then receive a 403 when attempting to sync (because the sync properly checks permissions). Added the ability to test for broadcasting and write matchers similar to how we test log messages. Previously we let 200 responses be delivered automatically by not setting a status code on the response writer, but this skips a log statement that is useful for request timing. As I go, I'm adding a 200 response to any handlers that are missing them for the sake of the log. Added more tests.
When bulkUpdate gets a query and the query fails for any reason (bad query, ES down, etc) the request responds with a 200 and an empty body. It should be a 500 with the usual "did not complete" message. Fixed.
Reorganized a little. Repeat Log Entry Matchers were moved to package level variables because they're stateless and can be reused. Added a default case in the Response object type check. This will help against a test appearing to pass if a test expects a type that the switch didn't account for.
Update docs to reference 404 failure in UpdateDetection endpoint. Account for unexpected error when calling UpdateDetection. Added table test case to cover it.
coreyogburn
force-pushed
the
cogburn/handler-tests
branch
from
df832bc to
88033ce
Compare
jertel
approved these changes
Jan 16, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By filling the entire server.Server with mocked resources, we can test the handlers specifically. These tests prioritize testing the inputs and outputs of the handlers: request bodies, url parameters, status code, response body, and any web socket broadcasts that might happen. This should help us keep our documentation honest about what results are returned under what circumstances. These changes also includes tests for what gets logged but because server resources, like Detection Engines, are mocked the logs are incomplete from what an a real request might generate. That being said, we now have the tools to update existing tests to validate what these resources would log.
Over time I'll be adding other handlers but for now this tests every Detection endpoint.
There are also a few bug fixes such as "Not Found" errors were returning 500s instead of 404s in some places and vice versa, endpoints that didn't explicitly respond with a 200 were missing a log statement that the handler executed successfully (along with important meta data like execution time), and a few places where we weren't checking permissions properly.