Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky test testAssembleQueryConfigUrl. #4083

Merged

Conversation

yyfMichaelYan
Copy link
Contributor

Fix flaky test testAssembleQueryConfigUrl due to the non-deterministic iteration order of HashMap.

This flaky test is found by using Nondex, a tool used to find flaky tests in Java projects. The assertion on Line#361 in RemoteConfigRepositoryTest.java may fail because the order of two entries, someKey and anotherKey, is non-deterministic, so the converted string of queryConfigUrl is non-deterministic, resulting in the flaky test. To fix this test, the data structure of the details field of ApolloNotificationMessages is changed from HashMap to TreeMap so that the entries of the map is sorted. As a result, the iteration order is deterministic and the test will always pass.

@nobodyiam
Copy link
Member

Thanks for the report. In this case, though the converted string of queryConfigUrl is non-deterministic due to hash map characteristic, I think this test will always pass as the actual logic to handle notificationMessages in assembleQueryConfigUrl is the same as testAssembleQueryConfigUrl.

assembleQueryConfigUrl:

    if (remoteMessages != null) {
      queryParams.put("messages", queryParamEscaper.escape(GSON.toJson(remoteMessages)));
    }

testAssembleQueryConfigUrl:

    assertTrue(queryConfigUrl
        .contains("messages=" + UrlEscapers.urlFormParameterEscaper()
            .escape(gson.toJson(notificationMessages))));

@yyfMichaelYan
Copy link
Contributor Author

Thanks for your response! Yes, both assembleQueryConfigUrl and testAssembleQueryConfigUrl use the same method toJson to convert the Object to JSON. However, the returned JSON may not always be the same every time toJson is called. If you check the implementation of toJson, you will find in the end that it needs to iterate the HashMap to convert it to JSON, and the iteration order is non-deterministic. In other words, it's possible that entryA comes before entryB the first time toJson is called, and entryB comes before entryA the second time toJson is called.

@codecov-commenter
Copy link

Codecov Report

Merging #4083 (8f4486a) into master (b00f238) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4083   +/-   ##
=========================================
  Coverage     52.45%   52.45%           
  Complexity     2612     2612           
=========================================
  Files           484      484           
  Lines         15203    15203           
  Branches       1572     1572           
=========================================
  Hits           7974     7974           
- Misses         6674     6675    +1     
+ Partials        555      554    -1     
Impacted Files Coverage Δ
...rk/apollo/core/dto/ApolloNotificationMessages.java 0.00% <0.00%> (ø)
...o/openapi/filter/ConsumerAuthenticationFilter.java 94.11% <0.00%> (-5.89%) ⬇️
...ervice/service/ReleaseMessageServiceWithCache.java 87.05% <0.00%> (+1.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b00f238...8f4486a. Read the comment docs.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nobodyiam nobodyiam merged commit 02fff62 into apolloconfig:master Nov 12, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2021
@nobodyiam nobodyiam added this to the 2.0.0 milestone Jan 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants