kaiyaok2 opened a new pull request, #929: URL: https://github.com/apache/wicket/pull/929
Fixes https://issues.apache.org/jira/browse/WICKET-7119 # The Problem Some unit tests are non-idempotent, as they pass in the first run but fail in the second run in the same environment. A fix is necessary since unit tests shall be self-contained, ensuring that the state of the system under test is consistent at the beginning of each test. In practice, fixing non-idempotent tests can help proactively avoid state pollution that results in test order dependency (which could cause problems under test selection, prioritization or parallelization). # Reproduce Using the `NIOInspector` plugin that supports rerunning JUnit tests in the same environment. Use `org.apache.wicket.util.string.StringsTest#stripJSessionId` as an example: ``` cd wicket-util mvn edu.illinois:NIOInspector:rerun -Dtest=org.apache.wicket.util.string.StringsTest#stripJSessionId ``` # 5 Non-Idempotent Tests & Proposed Fix ## org.apache.wicket.util.string.StringsTest#stripJSessionId Reason: The `SESSION_ID_PARAM` field in `Strings`(see https://github.com/apache/wicket/blob/37c80e8c54533257c9fc5737424847a61e889b4e/wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java#L74) contains semicolon by default, but `stripJSessionId()` resets it to `"jsessionid"` (see https://github.com/apache/wicket/blob/37c80e8c54533257c9fc5737424847a61e889b4e/wicket-util/src/test/java/org/apache/wicket/util/string/StringsTest.java#L62) when doing cleanup. As a result, `assertEquals(url, Strings.stripJSessionId(url + ";jsessionid=12345"));` will fail in the second test execution due to the missing semicolon. Error message of the test in the repeated run: ``` org.opentest4j.AssertionFailedError: expected: <http://localhost/abc> but was: <http://localhost/abc;> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177) at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145) at org.apache.wicket.util.string.StringsTest.stripJSessionId(StringsTest.java:43) ``` Fix: Save the original value of the field `SESSION_ID_PARAM` before modifying it, and restore it to the saved value at the end of the test. ## org.apache.wicket.resource.FileSystemResourceReferenceTest#testFileSystemResourceReferenceWithZip Reason: `FileSystemAlreadyExistsException` occurs when calling `FileSystemResourceReference.getPath()` because the file system for the zip file is already mounted when the test is executed a second time. Error message in the repeated run: ``` java.nio.file.FileSystemAlreadyExistsException: null at jdk.zipfs/jdk.nio.zipfs.ZipFileSystemProvider.newFileSystem(ZipFileSystemProvider.java:104) at java.base/java.nio.file.FileSystems.newFileSystem(FileSystems.java:339) at java.base/java.nio.file.FileSystems.newFileSystem(FileSystems.java:288) at org.apache.wicket.resource.FileSystemJarPathService.getPath(FileSystemJarPathService.java:75) at org.apache.wicket.resource.FileSystemResourceReference.getPath(FileSystemResourceReference.java:180) at org.apache.wicket.resource.FileSystemResourceReference.getPath(FileSystemResourceReference.java:204) at org.apache.wicket.resource.FileSystemResourceReferenceTest.testFileSystemResourceReferenceWithZip(FileSystemResourceReferenceTest.java:58) ``` Fix: Check if the file system for the zip file is already available before creating a new one. ## org.apache.wicket.markup.repeater.AbstractPageableViewTest#cachedItemCount Reason: `View.getItemCount()` calls `View.internalGetItemCount()`, which is dependent on the static variable `count`. The test `cachedItemCount()` resets `count` to 8 without resetting it, so in the second test execution `view.getItemCount()` would be 8 instead of 5. Error message of one of the tests in the repeated run: ``` org.opentest4j.AssertionFailedError: expected: <5> but was: <8> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:166) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:161) at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:632) at org.apache.wicket.markup.repeater.AbstractPageableViewTest.cachedItemCount(AbstractPageableViewTest.java:44) ``` Fix: Add a `@BeforeEach` method to ensure `count` is reset to 5. ## org.apache.wicket.ComponentOnConfigureTest#order Reason: Similarly, the static atomic integers are not reset before / after the test, so they get accumulated across multiple test runs, causing assertion failures. Error message of one of the tests in the repeated run: ``` org.opentest4j.AssertionFailedError: Page must be configured first! ==> expected: <0> but was: <6> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150) at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:563) at org.apache.wicket.ComponentOnConfigureTest.order(ComponentOnConfigureTest.java:54) ``` Fix: Add a `@BeforeEach` method to ensure the static atomic integers are reset to 0. ## org.apache.wicket.pageStore.AbstractPersistentPageStoreTest#rebindingAttributeDoesNotRemoveAllPages Reason: The constructor of `AbstractPersistentPageStore` checks if the input application name string already exists in the static concurrent map `STORES` (this would be the case in the second test execution), and throws `IllegalStateException` if so. Error message of one of the tests in the repeated run: ``` java.lang.IllegalStateException: Store with key 'fooBar:' already exists. at org.apache.wicket.pageStore.AbstractPersistentPageStore.<init>(AbstractPersistentPageStore.java:67) at org.apache.wicket.pageStore.AbstractPersistentPageStoreTest$1.<init>(AbstractPersistentPageStoreTest.java:44) at org.apache.wicket.pageStore.AbstractPersistentPageStoreTest.rebindingAttributeDoesNotRemoveAllPages(AbstractPersistentPageStoreTest.java:43) ``` Fix: Call `store.destroy()` to delete `'fooBar'` from the `STORES` map. # Verifying this change After the patch, running the tests repeatedly in the same environment will not lead to failures. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
