[
https://issues.apache.org/jira/browse/WICKET-7119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868419#comment-17868419
]
ASF GitHub Bot commented on WICKET-7119:
----------------------------------------
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.
> 5 unit tests are non-idempotent (passes in the first run but fails in
> subsequent runs in the same environment)
> --------------------------------------------------------------------------------------------------------------
>
> Key: WICKET-7119
> URL: https://issues.apache.org/jira/browse/WICKET-7119
> Project: Wicket
> Issue Type: Bug
> Reporter: Kaiyao Ke
> Priority: Major
>
> List of non-idempotent tests:
> org.apache.wicket.util.string.StringsTest#stripJSessionId
> org.apache.wicket.resource.FileSystemResourceReferenceTest#testFileSystemResourceReferenceWithZip
> org.apache.wicket.markup.repeater.AbstractPageableViewTest#cachedItemCount
> org.apache.wicket.ComponentOnConfigureTest#order
> org.apache.wicket.pageStore.AbstractPersistentPageStoreTest#rebindingAttributeDoesNotRemoveAllPages
> Will open a PR to fix them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
