-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Make Dispatchers.setMain affect all delays in tests. #3049
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
Conversation
…itialization cycle and add a test that ensures this behaviour in the future Fixes #3044
bd9009f
to
7d1cd38
Compare
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.
I'm a bit concerned this is way too intrusive change with potentially non-trivial interactions with existing/running code (e.g. now the source of default delay can be changed on the fly).
If you do not have objections, I would prefer sticking with option 1 for the initial release
I would also prefer the first option initially, at least until we deal with the initial feedback on both the updated test framework and the change to the default delay. Otherwise, the changes could just be too overwhelming: it's not easy now to diagnose the situation where the default delay is not used for some reason, and the consequences of entering such a situation would become much more significant. That said, I'd argued that, in code that's heavily using |
#3094 contains some examples of when delay-skipping is undesirable in tests. |
This might cause similar issues as in #3173, with actual test infrastructure delays being controlled by the test dispatcher. Perhaps this can be addressed by using another delay-handling dispatcher to run delays in the test infrastructure, as planned by the Turbine library. Note that as described in #3173, |
This PR can be now closed, can't it? |
Sure. |
Option 2.