Add unit and integration tests for untested web layer classes#31
Add unit and integration tests for untested web layer classes#31devin-ai-integration[bot] wants to merge 1 commit into
Conversation
New test files: - OpenmrsFilterTest: 11 tests for servlet filter behavior, UserContext lifecycle, cache headers - ListenerTest: 14 tests for static methods, state management, error handling - DispatcherServletTest: 2 tests for constructor and stopAndCloseApplicationContext - StaticDispatcherServletTest: 2 tests for constructor and stopAndCloseApplicationContext - OpenmrsBindingInitializerTest: 34 tests for property editor registration - JspClassLoaderFilterTest: 5 tests for classloader management - GZIPResponseWrapperTest: 15 tests for GZIP response wrapper - GZIPResponseStreamTest: 15 tests for GZIP response stream Total: 98 new tests across 8 test files Co-Authored-By: Dillon Vargo <dillonvargo@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| void getRuntimeProperties_shouldReturnNonNullResult() { | ||
| // getRuntimeProperties delegates to OpenmrsUtil.getRuntimeProperties | ||
| // It may return null if no runtime properties file is found, but should not throw | ||
| Listener.getRuntimeProperties(); | ||
| } |
There was a problem hiding this comment.
🟡 Test name claims non-null assertion but has no assertion at all
The test getRuntimeProperties_shouldReturnNonNullResult claims to verify a non-null return value, but contains no assertion whatsoever — it only calls Listener.getRuntimeProperties() without checking the result. The comment on line 138 even acknowledges "It may return null", directly contradicting the test name. This means the test will pass regardless of the return value (null or non-null), giving a false sense that the non-null contract is verified. If the intended behavior is to verify no exception is thrown, the test should be renamed (e.g., shouldNotThrow). If non-null is expected, assertNotNull should be added.
| void getRuntimeProperties_shouldReturnNonNullResult() { | |
| // getRuntimeProperties delegates to OpenmrsUtil.getRuntimeProperties | |
| // It may return null if no runtime properties file is found, but should not throw | |
| Listener.getRuntimeProperties(); | |
| } | |
| void getRuntimeProperties_shouldNotThrowException() { | |
| // getRuntimeProperties delegates to OpenmrsUtil.getRuntimeProperties | |
| // It may return null if no runtime properties file is found, but should not throw | |
| Listener.getRuntimeProperties(); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Description of what I changed
Added 8 new test files covering previously untested web layer classes, totaling 98 new tests:
OpenmrsFilterTestListenerTestruntimePropertiesFound,setupNeeded,openmrsStarted), error handling,contextDestroyedDispatcherServletTeststopAndCloseApplicationContextnull-safetyStaticDispatcherServletTeststopAndCloseApplicationContextnull-safetyOpenmrsBindingInitializerTestWebDataBinderJspClassLoaderFilterTestOpenmrsClassLoader, restoration behavior on exceptionGZIPResponseWrapperTestfinishResponse,flushBuffer,setContentLengthno-opGZIPResponseStreamTestNote on DispatcherServlet tests: Spring 6 marks
FrameworkServlet.getWebApplicationContext()asfinal, preventing mock-based testing of most servlet behavior via subclass overrides. Tests are limited to constructor andstopAndCloseApplicationContextnull-handling.Things for reviewers to look at closely
ListenerTestusesReflectionTestUtils.setFieldto manipulate private static fields — fragile if field names are refactored. ThecontextDestroyedtest swallows exceptions from an uninitializedContext, so it only verifies theopenmrsStartedflag transition.GZIPResponseWrapperTest.sendError_shouldSetErrorCodehas a weak assertion (assertNotNull(wrapper)) — it doesn't actually verify the error code was stored.OpenmrsBindingInitializerTestextendsBaseWebContextSensitiveTest(requires Spring context/DB). Each of the 34 tests checks a single type registration individually rather than using parameterized tests.Issue I worked on
N/A — test coverage improvement for the web module.
Checklist: I completed these to help reviewers :)
mvn clean packageright before creating this pull request and added all formatting changes to my commit.Link to Devin session: https://app.devin.ai/sessions/2d842dff86fe4872ac461c051bb01f16
Requested by: @dillonvargo