fix flaky tests in ParametersTest

[qz0610]

Description

One flaky test shouldAllowNullValues is found using Nondex. This PR now fixes it without introducing new plugins / dependencies. The test was found to be flaky when running the following command:

mvn -pl gremlin-core \
    edu.illinois:nondex-maven-plugin:2.1.7:nondex \
-Dtest=org.apache.tinkerpop.gremlin.process.traversal.step.util.ParametersTest#shouldAllowNullValues \
-Drat.numUnapprovedLicenses=200

The params array contains retrieved keys and value from a HashMap, that may lead to inconsistent test results due to the non-deterministic ordering of elements in a HashMap.

Proposed Fix

The expected keys and values are now stored in a new array 'expected'. Sort both 'params' and 'expected'. The comparison between 'params' and 'expected' has consistent results.

[Cole-Greer]

Thanks @qz0610 for finding this flaky test and submitting a fix. I have one concern with the solution which is that the array returned by parameters.getKeyValues() isn't entirely unordered. It is built up by iterating through a HashMap which isn't ordered, but each pair of elements in the resulting array is an ordered key-value pair. I think it's important that the test still asserts that key-value matching does not get scrambled.

For example new Object[] {"a", null, "b", "bat", "c", "cat"} and new Object[] {"b", "bat", "c", "cat", "a", null} should both be valid results, but new Object[] {"a", "bat" , "b", "cat", "c", null} is not valid.

Perhaps we could reconstruct the returned parameters back into a HashMap and assert equality on that to ensure the key-value pairing remains correct.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.47%. Comparing base (2d32517) to head (7672782).
Report is 349 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2820      +/-   ##
============================================
+ Coverage     76.16%   76.47%   +0.31%     
- Complexity    13170    13934     +764     
============================================
  Files          1085     1076       -9     
  Lines         65189    63324    -1865     
  Branches       7289     7457     +168     
============================================
- Hits          49651    48428    -1223     
+ Misses        12830    12367     -463     
+ Partials       2708     2529     -179     

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

[qz0610]

Thank you @Cole-Greer for pointing out my overlook of the reservation of key-value pairs, and the suggestion of reconstructing a HashMap. I updated the fix by comparing the expected HashMap with the one reconstructed from params.

[kenhuuu]

I'm curious, wouldn't this same problem affect other tests in that class like shouldGetKeyValues() and shouldGetKeyValuesIgnoringSomeKeys()

[qz0610]

Hi @kenhuuu, thank you for the insight. You are absolutely correct. This same problem also affects shouldGetKeyValues() and shouldGetKeyValuesIgnoringSomeKeys() in ParametersTest. I updated the fix to include fixing these two tests. After the fix, all tests in ParametersTest have no more failures with nondex configuration.

[kenhuuu]

I think it would have been a little nicer if a short helper method was used to create those Maps but it's OK since this is just a test file.

VOTE +1.

[Cole-Greer]

Thanks for all of the updates to the tests. Normally we ask that a PR such as this would be targeted to the 3.6-dev branch, our standard process is to target the oldest release branch which a change should be included in, and then whoever merges the PR will ensure the changes are merged up to all future branches. In this case I will cherry-pick the fix back to 3.6-dev and 3.7-dev branches.

VOTE +1 (merging as a CTR)