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

Bugfix: replace countdown latches with awaitility #1506

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Jun 21, 2022

What this PR changes/adds

Fixes a race condition in the ContractNegotiationIntegrationTest by replacing the CountdownLatch with awaitility.
Awaitility allows for better control over timed assertions than a latch - here it allows us to assert multiple things, in particular:

  • the listener's pre* method has been called
  • both negotiation managers are in the desired state

The latch only waited for the first condition, whereas Awaitility waits for all of them.

Why it does that

To avoid race conditions, causing sporadically failing tests.

Further notes

  • countdown latches have been replaced only in the ContractNegotiationIntegrationTest
  • replacing was also done in the @Disabled tests (why are they disabled?)

Linked Issue(s)

Closes #1505

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@paullatzelsperger paullatzelsperger changed the title replaced countdown latches with awaitility Bugfix: replace countdown latches with awaitility Jun 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

Merging #1506 (cb41670) into main (147ae2f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1506   +/-   ##
=======================================
  Coverage   67.32%   67.32%           
=======================================
  Files         725      725           
  Lines       16120    16120           
  Branches     1054     1054           
=======================================
  Hits        10853    10853           
  Misses       4785     4785           
  Partials      482      482           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 147ae2f...cb41670. Read the comment docs.

var providerNegotiation = providerStore.findForCorrelationId(consumerNegotiationId);

// Assert that provider and consumer have the same offers stored
assertThat(consumerNegotiation).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

seems much duplicate code?

Copy link
Member Author

Choose a reason for hiding this comment

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

i extracted a method that performs some basic assertions, but I'm not sure whether that makes the code more or less readable.

@paullatzelsperger paullatzelsperger force-pushed the bugfix/1505_negotiation_race_condition branch from ce4c660 to cb41670 Compare June 22, 2022 05:15
@paullatzelsperger paullatzelsperger merged commit a1ec87b into eclipse-edc:main Jun 22, 2022
@juliapampus juliapampus added this to In progress in Connector via automation Jun 24, 2022
@juliapampus juliapampus moved this from In progress to Done in Connector Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Connector
  
Done
Development

Successfully merging this pull request may close these issues.

ContractNegotiationIntegrationTest: race condition
3 participants