[
https://issues.apache.org/jira/browse/TINKERPOP-3079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17847194#comment-17847194
]
ASF GitHub Bot commented on TINKERPOP-3079:
-------------------------------------------
kaiyaok2 opened a new pull request, #2608:
URL: https://github.com/apache/tinkerpop/pull/2608
Fixes https://issues.apache.org/jira/browse/TINKERPOP-3079
### Brief Description of the Bug
The test `TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache`
is non-idempotent, as it passes in the first run but fails in the second run in
the same environment. The source of the problem is that the initial strategies
registration (`StrategyA`, `StrategyB` and `StrategyC`) for the `TestGraph` and
` TestGraphComputer` classes are static (see
https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L132
and
https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L185).
These static blocks will only be called once during class loading.
In the first execution of the test
`TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache`, `StrategyA`
is removed from global
strategies(https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L82),
and the test does not restore it after execution.
Therefore, in the second execution of the test, `StrategyA` is not present
in the global strategies, so assertions like
`assertTrue(strategies.getStrategy(StrategyA.class).isPresent())` would
fail(https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L77).
A fix is necessary since unit tests shall be self-contained. Idempotent
tests help maintain this isolation by ensuring that the state of the system
under test is consistent at the beginning of each test, regardless of previous
test runs. For example, fixing non-idempotent tests can help proactively avoid
state pollution that results in test order dependency (which could hurt
regression testing with the use of test selection / prioritization /
parallelism.
### Failure Message in the 2nd Test Run:
```
java.lang.AssertionError:
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at
org.apache.tinkerpop.gremlin.process.TraversalStrategiesTest.shouldAllowUserManipulationOfGlobalCache(TraversalStrategiesTest.java:77)
```
### Reproduce
Use the `NIOInspector` plugin that supports rerunning individual tests in
the same environment:
```
cd gremlin-core
mvn edu.illinois:NIOInspector:rerun
-Dtest=org.apache.tinkerpop.gremlin.process.TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache
```
### Proposed Fix
Handle initial strategies registration in a `setup()` method rather than in
static blocks of test classes.
### Verify the Change
After the patch, all tests pass idempotently.
> The test `TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache`
> is not idempotent, as it passes in the first run and fails in repeated runs
> in the same environment.
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: TINKERPOP-3079
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3079
> Project: TinkerPop
> Issue Type: Bug
> Environment: Ubuntu 22.04, Java 17
> Reporter: Kaiyao Ke
> Priority: Major
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> ### Brief Description of the Bug
> The test `TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache`
> is non-idempotent, as it passes in the first run but fails in the second run
> in the same environment. The source of the problem is that the initial
> strategies registration (`StrategyA`, `StrategyB` and `StrategyC`) for the
> `TestGraph` and ` TestGraphComputer` classes are static (see
> https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L132
> and
> https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L185).
> These static blocks will only be called once during class loading.
> In the first execution of the test
> `TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache`,
> `StrategyA` is removed from global
> strategies(https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L82),
> and the test does not restore it after execution.
> Therefore, in the second execution of the test, `StrategyA` is not present in
> the global strategies, so assertions like
> `assertTrue(strategies.getStrategy(StrategyA.class).isPresent())` would
> fail(https://github.com/apache/tinkerpop/blob/66e5a47ffd3976d29bc7797c399b8e11c5ba810e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/TraversalStrategiesTest.java#L77).
> A fix is necessary since unit tests shall be self-contained. Idempotent tests
> help maintain this isolation by ensuring that the state of the system under
> test is consistent at the beginning of each test, regardless of previous test
> runs. For example, fixing non-idempotent tests can help proactively avoid
> state pollution that results in test order dependency (which could hurt
> regression testing with the use of test selection / prioritization /
> parallelism.
> ### Failure Message in the 2nd Test Run:
> ```
> java.lang.AssertionError:
> at org.junit.Assert.fail(Assert.java:87)
> at org.junit.Assert.assertTrue(Assert.java:42)
> at org.junit.Assert.assertTrue(Assert.java:53)
> at
> org.apache.tinkerpop.gremlin.process.TraversalStrategiesTest.shouldAllowUserManipulationOfGlobalCache(TraversalStrategiesTest.java:77)
> ```
> ### Reproduce
> Use the `NIOInspector` plugin that supports rerunning individual tests in the
> same environment:
> ```
> cd gremlin-core
> mvn edu.illinois:NIOInspector:rerun
> -Dtest=org.apache.tinkerpop.gremlin.process.TraversalStrategiesTest#shouldAllowUserManipulationOfGlobalCache
> ```
> ### Proposed Fix
> Handle initial strategies registration in a `setup()` method rather than in
> static blocks of test classes.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
