-
Notifications
You must be signed in to change notification settings - Fork 15
Upgrade to newer Awaitility. #2429
Upgrade to newer Awaitility. #2429
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, some small comments.
@@ -146,7 +146,7 @@ | |||
public void setup() throws JsonProcessingException { | |||
// Change code to run synchronously, but with a timeout in case something's gone horribly wrong | |||
originalAsyncMethod = TransactionManagers.runAsync; | |||
TransactionManagers.runAsync = task -> Awaitility.await().atMost(2, TimeUnit.SECONDS).until(task); | |||
TransactionManagers.runAsync = task -> Awaitility.await().atMost(2, TimeUnit.SECONDS).untilAsserted(task::run); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
task
is a Runnable I believe, why did we change this to task::run
?
@@ -9,7 +9,7 @@ com.google.dagger:dagger = 2.0.2 | |||
com.google.dagger:dagger-compiler = 2.0.2 | |||
com.google.guava:* = 18.0 | |||
com.googlecode.json-simple:json-simple = 1.1.1 | |||
com.jayway.awaitility:awaitility = 1.6.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the release notes, https://github.com/awaitility/awaitility/wiki/ReleaseNotes30. The changes make sense. Thanks for the contribution!
Closing in favor of #2668. |
Whoops, I somehow missed that there had been responses. task is indeed Runnable, but untilAsserted demands a ThrowingRunnable. |
Goals (and why):
Implementation Description (bullets):
Concerns (what feedback would you like?):
Where should we start reviewing?:
Priority (whenever / two weeks / yesterday):
This change is