Enhance property clearing in StableMockExtension to prevent stale pro… #53
Conversation
…perties - Added logic to clear class-scoped indexed properties in addition to the existing indexed properties. - This change ensures that stale properties from previous test runs do not cause flaky tests.
There was a problem hiding this comment.
Pull request overview
This PR enhances property cleanup in the StableMockExtension.afterAll() method to prevent stale properties from previous test runs that could cause flaky tests. The key improvement is unconditionally clearing indexed properties and adding cleanup for class-scoped indexed properties.
Key Changes:
- Removed conditional check for clearing indexed properties (now always cleared since they're always set)
- Added cleanup of class-scoped indexed properties to match the properties set during initialization
- Improved code comments to explain the rationale for the cleanup logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String testClassName = TestContextResolver.getTestClassName(context); | ||
| for (int i = 0; i < servers.size(); i++) { | ||
| WireMockServer server = servers.get(i); | ||
| if (server != null) { | ||
| server.stop(); | ||
| } | ||
| if (StableMockConfig.useGlobalProperties()) { | ||
| System.clearProperty(StableMockConfig.PORT_PROPERTY + "." + i); | ||
| System.clearProperty(StableMockConfig.BASE_URL_PROPERTY + "." + i); | ||
| } | ||
| // Always clear indexed properties since they're always set (needed for repeatable annotations) | ||
| // This prevents stale properties from previous test runs causing flaky tests | ||
| System.clearProperty(StableMockConfig.PORT_PROPERTY + "." + i); | ||
| System.clearProperty(StableMockConfig.BASE_URL_PROPERTY + "." + i); | ||
| // Also clear class-scoped indexed properties | ||
| System.clearProperty(StableMockConfig.PORT_PROPERTY + "." + testClassName + "." + i); | ||
| System.clearProperty(StableMockConfig.BASE_URL_PROPERTY + "." + testClassName + "." + i); |
There was a problem hiding this comment.
The cleanup is incomplete and doesn't fully prevent stale properties. While this code correctly clears the indexed properties and class-scoped indexed properties, it's missing the cleanup of class-scoped non-indexed properties that are set in beforeAll.
In beforeAll (lines 128-129 for multi-URL and lines 189-190 for single-URL), these properties are always set:
StableMockConfig.PORT_PROPERTY + "." + testClassNameStableMockConfig.BASE_URL_PROPERTY + "." + testClassName
These properties should also be cleared in afterAll to prevent stale values from affecting subsequent test runs. Since these properties are set regardless of whether servers is null/empty, they should be cleared outside this if block (similar to how the global properties are cleared at lines 630-633). Move the testClassName retrieval before the if statement and add the cleanup of these class-scoped non-indexed properties after line 633.
…erManager - Added increased delays in both classes to ensure proper port release, particularly in CI environments. - Implemented progressive delays during server startup retries to enhance reliability and reduce test flakiness.
- Introduced a new method `waitForPortRelease` to ensure ports are fully released after stopping servers, addressing potential conflicts in parallel test execution. - Removed fixed sleep delays in favor of a more robust approach that checks port availability, enhancing reliability in CI environments.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Port is available, close immediately | ||
| socket.close(); |
There was a problem hiding this comment.
Redundant socket.close() call detected. The try-with-resources statement already ensures the socket is closed automatically when exiting the try block. Calling socket.close() explicitly at line 671 is unnecessary and could potentially cause issues if an exception occurs between binding and the explicit close call.
| // Port is available, close immediately | |
| socket.close(); | |
| // Port is available; will be closed automatically by try-with-resources |
| // Port is available, close immediately | ||
| socket.close(); | ||
| return; // Port is released |
There was a problem hiding this comment.
The comment at line 670 is misleading. The socket is already automatically closed by the try-with-resources statement at line 667, not by the explicit close() call at line 671. Either remove the explicit close() call or update the comment to accurately reflect that the try-with-resources handles the closing.
| // Port is available, close immediately | |
| socket.close(); | |
| return; // Port is released | |
| // Port is available; socket will be closed by try-with-resources | |
| return; // Port is released |
- Moved the retrieval of the test class name to the beginning of the `afterAll` method for early access during cleanup. - Enhanced property clearing logic to ensure class-scoped non-indexed properties are cleared, preventing stale properties from affecting subsequent test runs. - Updated comments for clarity regarding resource management.
- Added logic to handle JAR URLs for locating test resources, addressing potential FileNotFoundExceptions in JDK 17. - Implemented a method to find the project root from a JAR file path, improving fallback resource directory detection. - Introduced a fallback mechanism for resource directory retrieval when the primary method fails.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 105 out of 147 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…perties
Description
Brief description of the changes in this PR.
Type of Change
Related Issue
Fixes #(issue number)
Changes Made
Testing
Checklist
Screenshots (if applicable)
Add screenshots to help explain your changes.
Additional Notes
Any additional information that reviewers should know.