Flaky test fields iteration order #4095
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4095 +/- ##
=========================================
Coverage 52.49% 52.49%
+ Complexity 2614 2613 -1
=========================================
Files 484 484
Lines 15206 15206
Branches 1572 1572
=========================================
Hits 7982 7982
Misses 6671 6671
Partials 553 553
Continue to review full report at Codecov.
|
| @@ -510,7 +510,10 @@ public void testAssembleLongPollRefreshUrl() throws Exception { | |||
| assertTrue(longPollRefreshUrl.contains("cluster=someCluster%2B+%26.-_someSign")); | |||
| assertTrue(longPollRefreshUrl.contains( | |||
| "notifications=%5B%7B%22namespaceName%22%3A%22" + someNamespace | |||
| + "%22%2C%22notificationId%22%3A" + 1 + "%7D%5D")); | |||
| + "%22%2C%22notificationId%22%3A" + 1 + "%7D%5D") | |||
There was a problem hiding this comment.
Will it make things easy if we make the notificationsMap treemap?
There was a problem hiding this comment.
It will not work by changing the implementation of notificationsMap to TreeMap. The iteration order of ImmutableMap is deterministic, which is why in the assertion of testAssembleLongPollRefreshUrlWithMultipleNamespaces, someName always comes before anotherName. In other words, these two flaky tests are not caused by the iteration order of entries of the map. However, for either someName and anotherName, the order of the two fields, namespace and notificationId, is non-deterministic. This is why all 4 possible combinations are covered in the assertTrue in testAssembleLongPollRefreshUrlWithMultipleNamespaces.
There was a problem hiding this comment.
I got your point, it's not the map but the gson.toJson that might make the fields in different order.
I checked the com.google.gson.internal.bind.ReflectiveTypeAdapterFactory#getBoundFields method, and it seems the order is determined by the java.lang.Class#getDeclaredFields.
There was a problem hiding this comment.
Thanks for your information! Yes. I also went through all implementations of the write method which is hidden in the toJson method. I agree that the order may be determined by java.lang.Class#getDeclaredFields. However, since it's built in some package not maintained by this repository, I feel that it's better to change the assertion in our tests rather than modify the code of java.lang.Class#getDeclaredFields?
There was a problem hiding this comment.
I agree with you that the order might change in different execution environments. But the current changes to test all combinations of the URLs look quite complicated, which is not easy to maintain, e.g. if someone needs to add more namespaces to testAssembleLongPollRefreshUrlWithMultipleNamespaces.
Is there some way to make the assertions simpler while still could works when the order of fields is changed?
There was a problem hiding this comment.
Thanks! I just updated both assertions. The assertions now will check that every field is converted to string, but it doesn't check the order of the fields. The assertions are now less strict than before, because some characters between neighboring fields are not checked, such as {, }, and ,, but they look very simple and I think it's okay if we don't check those characters.
| + "%22%2C%22notificationId%22%3A" + someNotificationId | ||
| + "%7D%2C%7B%22namespaceName%22%3A%22" + anotherNamespace | ||
| + "%22%2C%22notificationId%22%3A" + anotherNotificationId + "%7D%5D")); | ||
| longPollRefreshUrl.contains("notifications=%5B%7B" + someNamespaceEntry + "%2C" + someNotificationIdEntry |
There was a problem hiding this comment.
Will it make things easy if we make the notificationsMap treemap?
…ongPollRefreshUrlWithMultipleNamespaces.
Fix two flaky tests, testAssembleLongPollRefreshUrl and testAssembleLongPollRefreshUrlWithMultipleNamespaces.
This flaky test is found by using Nondex, a tool used to find flaky tests in Java projects. The assertion on Line#511 of testAssembleLongPollRefreshUrl and Line#538 of testAssembleLongPollRefreshUrlWithMultipleNamespaces may fail because the iteration order of the two fields,
namespaceNameandnotificationId, of the classApolloConfigNotificationis non-deterministic in the method ofGSON.toJson. As a result, the resulting string may havenotificationIdbefore or afternamespaceName. Since theGSONpackage is not maintained in this repo, it's impossible to change the iteration order of the fields. Therefore, to fix the flaky tests, all possible iteration orders of the two fields are covered in the assertions.