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 SequencerServerTest Suite #3492

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Fix Flaky SequencerServerTest Suite #3492

merged 1 commit into from
Jan 18, 2023

Conversation

zfrenette
Copy link
Collaborator

@zfrenette zfrenette commented Jan 18, 2023

This patch addresses flakiness in the SequencerServerTest suite. Since the reportSequencerHealth method is executing on a separate thread, its periodic invocation can interfere with the mocked behaviour of other objects unpredictably. This issue is addressed by mocking the health report scheduler, making the tests deterministic.

Overview

Description:

Why should this be merged:

Related issue(s) (if applicable): #3460 #3487

Checklist (Definition of Done):

  • There are no TODOs left in the code
  • Coding conventions (e.g. for logging, unit tests) have been followed
  • Change is covered by automated tests
  • Public API has Javadoc

@zfrenette zfrenette self-assigned this Jan 18, 2023
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #3492 (b207ada) into master (ece3c8b) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #3492      +/-   ##
============================================
- Coverage     77.66%   77.57%   -0.09%     
+ Complexity     4950     4946       -4     
============================================
  Files           472      472              
  Lines         23621    23622       +1     
  Branches       2084     2084              
============================================
- Hits          18343    18323      -20     
- Misses         4253     4272      +19     
- Partials       1025     1027       +2     
Impacted Files Coverage Δ
...va/org/corfudb/infrastructure/SequencerServer.java 90.99% <100.00%> (+0.03%) ⬆️
...ain/java/org/corfudb/runtime/view/ObjectsView.java 79.55% <0.00%> (-6.82%) ⬇️
...nsport/sample/GRPCLogReplicationServerHandler.java 66.67% <0.00%> (-3.51%) ⬇️
...va/org/corfudb/util/serializer/JsonSerializer.java 43.10% <0.00%> (-3.45%) ⬇️
...org/corfudb/runtime/view/LayoutManagementView.java 78.29% <0.00%> (-3.43%) ⬇️
...untime/src/main/java/org/corfudb/util/CFUtils.java 85.00% <0.00%> (-3.33%) ⬇️
...lication/replication/send/SenderBufferManager.java 58.73% <0.00%> (-3.17%) ⬇️
...n/java/org/corfudb/infrastructure/CorfuServer.java 60.47% <0.00%> (-2.33%) ⬇️
...va/org/corfudb/infrastructure/ManagementAgent.java 75.00% <0.00%> (-2.17%) ⬇️
...orfudb/infrastructure/RemoteMonitoringService.java 93.30% <0.00%> (+0.56%) ⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This patch addresses flakiness in the SequencerServerTest suite.
Since the reportSequencerHealth method is executing on a separate
thread, its periodic invocation can interfere with the mocked
behaviour of other objects unpredictably. This issue is addressed by
mocking the health report scheduler, making the tests deterministic.
Copy link
Collaborator

@souravgarg souravgarg left a comment

Choose a reason for hiding this comment

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

That's neat. LGTM

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

5 participants