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 a flaky test #781

Merged
merged 1 commit into from
Oct 22, 2023
Merged

Fix a flaky test #781

merged 1 commit into from
Oct 22, 2023

Conversation

dserfe
Copy link
Contributor

@dserfe dserfe commented Oct 10, 2023

This PR is to fix a flaky test o.moquette.broker.MemoryRetainedRepositoryTest#testRetainedOnTopicReturnsWildcardTopicMatch in module broker.

Test failures

  • Run the following commands to reproduce the test failure:
 mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -pl broker -Dtest=io.moquette.broker.MemoryRetainedRepositoryTest#testRetainedOnTopicReturnsWildcardTopicMatch
  • Then we get the following test failures:
[ERROR]   MemoryRetainedRepositoryTest.testRetainedOnTopicReturnsWildcardTopicMatch:87 expected: <foo/bar/baz> but was: <foo/baz/bar>

Root cause

In line 84 of the test file, retainedMessages = repository.retainedOnTopic("foo/#"); calls retainedOnTopic from broker/src/main/java/io/moquette/broker/MemoryRetainedRepository.java.
In line 33 of the main class file provided, storage is defined as a ConcurrentHashMap, its keys are not in sorted order. Consequently, in line 58, when the retainedOnTopic method calls storage.entrySet(), the returned elements' order is non-deterministic. This leads to inconsistencies in the order of the elements in matchingMessages.
When it is returned to the test, the assertion in lines 87-88 (in the test file) assumes the orders are consistent, causing the test to fail.

Fix

To fix the flakiness, we can either change the test code or change the main code:

  1. One fix is to sort the elements of retainedMessages before making assertions of them in the test, which is the code change in this PR.
  2. If you prefer to resolve the flakiness from the main code, the fix is to use ConcurrentSkipListMap instead of ConcurrentMap. ConcurrentSkipListMap is sorted and thread-safe. If you prefer the code changes of the following PR: https://github.com/dserfe/moquette/pull/2/files, please let me know!

Copy link
Collaborator

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@andsel andsel merged commit 79053a3 into moquette-io:main Oct 22, 2023
4 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.

2 participants