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 non-deterministic test writer_readingMultipleTopics_shouldBatchSeparate() #28

Merged
merged 5 commits into from
Apr 22, 2024

Conversation

chessvivek
Copy link
Contributor

What does this PR do?

Hi, I am from the Software Testing research group at the University of Illinois. We found that the test com.datadoghq.connect.logs.sink.DatadogLogsApiWriterTest.writer_readingMultipleTopics_shouldBatchSeparate() is non deterministic or flaky. We use the NonDex tool to detect the non determinism (https://github.com/TestingResearchIllinois/NonDex). This PR highlights the source of non determinism and proposes a fix.

Reproduction steps:

  1. clone the repo and cd into it.
  2. mvn install -am
  3. mvn test -Dtest=com.datadoghq.connect.logs.sink.DatadogLogsApiWriterTest#writer_readingMultipleTopics_shouldBatchSeparateThis will run the test as is. Confirm that this passes.
  4. mvn edu.illinois:nondex-maven-plugin:1.1.2:nondex -DnondexMode=ONE -Dtest=com.datadoghq.connect.logs.sink.DatadogLogsApiWriterTest#writer_readingMultipleTopics_shouldBatchSeparate This command runs the same test with the NonDex tool. Notice that the build fails this time, which means that the test is non deterministic.

Source of non determinism:
The non determinism arises from line 62 in void com.datadoghq.connect.logs.sink.DatadogLogsApiWriter.flushBatches(). On that line, we are iterating on a HashMap. Depending on the iteration order, either of request1 or request2 may come first in the loop. This is because the HashMap interface is not guaranteed to have a deterministic order of iteration of its elements. From java documentation on HashMap: This class makes no guarantees as to the order of the map; in particular, it does not guarantee that the order will remain constant over time. (https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html). Therefore, our tests need to account for all the possibilities of iteration order. Currently, it only accounts for one of the order, and it only works because of the underlying specific implementation of HashMap, which may change anytime.

Proposed fix:
We account for both the iteration orders (request1 first or request2 first) in the test. You may see the files changed for the exact fix.

Motivation

What inspired you to submit this pull request?

At the Software Testing research group, one of our projects is to find and fix flaky tests in various repositories like this one. See our project repository at https://github.com/TestingResearchIllinois/idoft.

Additional Notes

Anything else we should know when reviewing?

@chessvivek chessvivek requested a review from a team as a code owner April 22, 2024 21:38
@jszwedko
Copy link
Contributor

Thanks for this @chessvivek and apologies for the very long delay in review. The changes make sense since you are correct that HashMap iteration order is non-deterministic.

@jszwedko jszwedko merged commit f9ca534 into DataDog:master Apr 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants