Apply review feedback: use class reference and add unit tests#203
Apply review feedback: use class reference and add unit tests#203
Conversation
Co-authored-by: buckett <5921+buckett@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves the Sentry “before send” filtering configuration by making the excluded logger reference refactor-safe and by extracting the filter logic into a testable method, with unit tests added to validate the behavior.
Changes:
- Replaced a hard-coded excluded logger string with
DefaultHandlerExceptionResolver.class.getName(). - Extracted filtering logic into a static
filterEvent(SentryEvent)helper for reuse/testability. - Added JUnit tests covering excluded logger, non-excluded logger, and null logger cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/uk/ac/ox/ctl/SentryLoggingFilterConfiguration.java | Uses a class-based logger name and routes callback logic through a testable filterEvent() method. |
| src/test/java/uk/ac/ox/ctl/SentryLoggingFilterConfigurationTest.java | Adds unit coverage for filtered vs pass-through behavior (including null logger). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void filterEvent_shouldReturnEvent_whenLoggerIsNotExcluded() { | ||
| SentryEvent event = new SentryEvent(); | ||
| event.setLogger("com.example.SomeOtherLogger"); | ||
|
|
||
| SentryEvent result = SentryLoggingFilterConfiguration.filterEvent(event); | ||
|
|
||
| assertNotNull(result, "Event should not be filtered when logger is not excluded"); | ||
| } |
There was a problem hiding this comment.
These tests only assert the result is non-null, but the method name implies the original event is passed through unchanged. Consider asserting the returned value is the same instance to better protect the intended behavior.
| void filterEvent_shouldReturnEvent_whenLoggerIsNull() { | ||
| SentryEvent event = new SentryEvent(); | ||
| event.setLogger(null); | ||
|
|
||
| SentryEvent result = SentryLoggingFilterConfiguration.filterEvent(event); | ||
|
|
||
| assertNotNull(result, "Event should not be filtered when logger is null"); | ||
| } |
There was a problem hiding this comment.
This test asserts non-null, but if the intended behavior is to pass the same event through when logger is null, assert the returned value is the same instance for a stronger regression check.
Applies review feedback from PR #202 to improve maintainability and test coverage of the Sentry logging filter configuration.
Changes:
DefaultHandlerExceptionResolver.class.getName()to prevent typos and stay in sync with refactoringfilterEvent()method for testability💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.