Fix Six Flaky Tests in /gremlin-core

[qz0610]

Description

Six flaky tests in gremlin-core were found using nondex:

org.apache.tinkerpop.gremlin.process.traversal.translator.DotNetTranslatorTest.shouldTranslateStrategies
org.apache.tinkerpop.gremlin.process.traversal.translator.JavascriptTranslatorTest.shouldTranslateStrategies
org.apache.tinkerpop.gremlin.process.traversal.translator.GolangTranslatorTest.shouldTranslateStrategies
org.apache.tinkerpop.gremlin.process.traversal.translator.PythonTranslatorTest.shouldTranslateStrategies
org.apache.tinkerpop.gremlin.process.traversal.translator.GroovyTranslatorTest.shouldTranslateStrategies
org.apache.tinkerpop.gremlin.language.grammar.TraversalStrategyVisitorTest.testTraversalStrategy

The flakiness can be reproduced by running:

mvn -pl gremlin-core \
    edu.illinois:nondex-maven-plugin:2.1.7:nondex \
-Dtest=[test_path] \
-Drat.skip=true

flaky test failures:

[ERROR]   TraversalStrategyVisitorTest.testTraversalStrategy:89 expected:<{logWarning=false, throwException=false, keys=[id, label]}> but was:<{keys=[label, id], throwException=false, logWarning=false}>
[ERROR]   TraversalStrategyVisitorTest.testTraversalStrategy:89 expected:<{logWarning=false, throwException=false, keys=[b, a]}> but was:<{keys=[a, b], logWarning=false, throwException=false}>
[ERROR]   TraversalStrategyVisitorTest.testTraversalStrategy:89 expected:<{throwException=true, keys=[id, label], logWarning=true}> but was:<{throwException=true, keys=[label, id], logWarning=true}>
[ERROR]   GroovyTranslatorTest.shouldTranslateStrategies:70 expected:<...ew SubgraphStrategy([checkAdjacentVertices: false, vertices: __.hasLabel("person")])).V().has("name")> but was:<...ew SubgraphStrategy([vertices: __.hasLabel("person"), checkAdjacentVertices: false])).V().has("name")>
[ERROR]   PythonTranslatorTest.shouldTranslateStrategies:98 expected:<...y('SeedStrategy',{'s[eed':999999,'strategy':'org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.SeedStrategy']}, 'org.apache.tinke...> but was:<...y('SeedStrategy',{'s[trategy':'org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.SeedStrategy','seed':999999]}, 'org.apache.tinke...>
[ERROR]   JavascriptTranslatorTest.shouldTranslateStrategies:60 expected:<...w SubgraphStrategy({[checkAdjacentVertices:false,vertices:__.hasLabel("person")]}),new SeedStrategy(...> but was:<...w SubgraphStrategy({[vertices:__.hasLabel("person"),checkAdjacentVertices:false]}),new SeedStrategy(...>
[ERROR]   GolangTranslatorTest.shouldTranslateStrategies:96 expected:<...graphStrategyConfig{[CheckAdjacentVertices: false, Vertices: gremlingo.T__.HasLabel("person")]}), gremlingo.SeedSt...> but was:<...graphStrategyConfig{[Vertices: gremlingo.T__.HasLabel("person"), CheckAdjacentVertices: false]}), gremlingo.SeedSt...>
[ERROR]   DotNetTranslatorTest.shouldTranslateStrategies:67 expected:<...ew SubgraphStrategy([checkAdjacentVertices: false, vertices: __.HasLabel("person")]),new SeedStrategy(s...> but was:<...ew SubgraphStrategy([vertices: __.HasLabel("person"), checkAdjacentVertices: false]),new SeedStrategy(s...>

The five shouldTranslateStrategies tests have the same root cause for flakiness: these tests assume the map returned bygetConfiguration() in subgraphStrategy class stores elements in deterministic order, that may result in inconsistent test results. Similarly, TraversalStrategyVisitorTest.testTraversalStrategy assumes the deterministic order of keys stored in HashSet.

Proposed Fix

This pr proposes a simple fix by storing the configuration of subgraphStrategy in LinkedHashMap, and storing the reserved keys in LinkedHashSet. After this fix, all the six tests pass consistently. Alternatively, if maintaining Hashmap and Hashset is preferred, I could try to modify the test assertions using regular expression matches or custom comparators.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.43%. Comparing base (9b46b67) to head (aafb363).
Report is 273 commits behind head on 3.7-dev.

Files with missing lines	Patch %	Lines
...verification/ReservedKeysVerificationStrategy.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             3.7-dev    #2887      +/-   ##
=============================================
+ Coverage      76.14%   76.43%   +0.28%     
- Complexity     13152    13226      +74     
=============================================
  Files           1084     1060      -24     
  Lines          65160    61491    -3669     
  Branches        7285     7336      +51     
=============================================
- Hits           49616    46999    -2617     
+ Misses         12839    11972     -867     
+ Partials        2705     2520     -185     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Cole-Greer]

Hi @qz0610, thanks for opening this PR. Overall the change looks good to me. Just a quick note, the 3.6-dev branch has not been reopened following the recent release of 3.6.8, as that will likely be the final release in the 3.6.x line. This topic was raised on the devlist last week (https://lists.apache.org/thread/ysc1ov4l3x3v3nyjr3b7z05tml7s7o46).

Would you mind re-targeting this PR to 3.7-dev?

[qz0610]

Hi @Cole-Greer, thank you for the review and feedback. The PR is now re-targeted to 3.7-dev. Please let me know if there's anything else I need to adjust.

[kenhuuu]

VOTE +1