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.

Sign up for GitHub

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

Flaky test fields iteration order #4095

Merged
merged 4 commits into from
Nov 26, 2021
Merged

Flaky test fields iteration order #4095

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f80148d
Fix flaky test testAssembleLongPollRefreshUrlWithMultipleNamespaces.
yyfMichaelYan Nov 12, 2021
5826436
Fix flaky test testAssembleLongPollRefreshUrl.
yyfMichaelYan Nov 12, 2021
b6d985e
Update assertions in testAssembleLongPollRefreshUrl and testAssembleL…
yyfMichaelYan Nov 25, 2021
21e1312
Merge branch 'master' into flakyTestFieldsIterationOrder
nobodyiam Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions ...t/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

Will it make things easy if we make the notificationsMap treemap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

|| longPollRefreshUrl.contains(
"notifications=%5B%7B%22notificationId%22%3A" + 1
+ "%2C%22namespaceName%22%3A%22" + someNamespace + "%22%7D%5D"));
}

@Test
Expand All @@ -532,11 +535,19 @@ public void testAssembleLongPollRefreshUrlWithMultipleNamespaces() throws Except
assertTrue(longPollRefreshUrl.contains(someServerUrl + "/notifications/v2?"));
assertTrue(longPollRefreshUrl.contains("appId=" + someAppId));
assertTrue(longPollRefreshUrl.contains("cluster=someCluster%2B+%26.-_someSign"));
String someNamespaceEntry = "%22namespaceName%22%3A%22" + someNamespace + "%22";
String someNotificationIdEntry = "%22notificationId%22%3A" + someNotificationId;
String anotherNamespaceEntry = "%22namespaceName%22%3A%22" + anotherNamespace + "%22";
String anotherNotificationIdEntry = "%22notificationId%22%3A" + anotherNotificationId;
assertTrue(
longPollRefreshUrl.contains("notifications=%5B%7B%22namespaceName%22%3A%22" + someNamespace
+ "%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
Copy link
Member

Choose a reason for hiding this comment

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

Will it make things easy if we make the notificationsMap treemap?

+ "%7D%2C%7B" + anotherNamespaceEntry + "%2C" + anotherNotificationIdEntry + "%7D%5D")
|| longPollRefreshUrl.contains("notifications=%5B%7B" + someNotificationIdEntry + "%2C" + someNamespaceEntry
+ "%7D%2C%7B" + anotherNotificationIdEntry + "%2C" + anotherNamespaceEntry + "%7D%5D")
|| longPollRefreshUrl.contains("notifications=%5B%7B" + someNotificationIdEntry + "%2C" + someNamespaceEntry
+ "%7D%2C%7B" + anotherNamespaceEntry + "%2C" + anotherNotificationIdEntry + "%7D%5D")
|| longPollRefreshUrl.contains("notifications=%5B%7B" + someNamespaceEntry + "%2C" + someNotificationIdEntry
+ "%7D%2C%7B" + anotherNotificationIdEntry + "%2C" + anotherNamespaceEntry + "%7D%5D"));
}

public static class MockConfigUtil extends ConfigUtil {
Expand Down