-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Support Async specs and BeforeAndAfter* hooks #255
base: main
Are you sure you want to change the base?
Conversation
this allows implementing specs to use either sync or async suites
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
wanted to get this up for discussion - this should allow users to make use of either sync or async tests, but there is one notable change in behavior: Using
with this change, it would show up as an aborted suite and the test run would fail:
This can be observed on the Travis CI builds for the PR - the Safari/IE tests were previously showing up as canceled due to being unable to create a webdriver - but are now aborted and failing the build (tests previously not working, but are now aborted->failure rather than cancelled->success) There could be different mindsets on whether that's a good or bad thing - but it is a change in behavior, so looking for feedback on whether that's acceptable before continuing on. If so, I think there could be a follow up to switch PerSuites over to use BeforeAndAfterAll, to address #111 |
I like the change, perhaps we should add a switch to disable tests that rely on webdriver so they can be explicitly cancelled when running on a system that doesn't have the browsers installed? |
that could work. also another option: it feels a bit hacky, but overriding testDataFor will allow the cancellation to keep the previous behavior
happy to either:
for now, any preference? |
for app and server fixtures. fixes playframework#111, and also doesn't start app/server if test is excluded via tags
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
for now, I've reverted the browser specs back, and added a round 2 on the app/server specs. so this PR in total, for app/server fixtures:
not sure what to do about the mima binary change errors |
another issue popped up - this currently breaks the previous behavior for using ParallelTestExecution with PerSuite, like before: results in an app/server being created per test (perhaps surprising behavior, was to us) what's the intended behavior? [resolved] - updated so that |
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
*/ | ||
final implicit def app: Application = synchronized { configuredApp } | ||
|
||
protected implicit lazy val runningServer: RunningServer = |
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.
it looked like this trait was previously starting up another running server, so replaced with the one from the Base suite
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
Not being allowed to use either sync or async suites has bothered me for so long. Oh I would so love to see this change outthere. |
I've signed the CLA (a few times) - don't know why it hasn't picked it up If someone can lend some guidance on what to do with the MIMA errors, would be happy to fix them up |
The signature for github user 'kwinter' was indeed recorded, but the commits have 'kyle.winter@bamtechmedia.com' as the email address for the |
Hmm, that looks pretty tricky: AFAICS these changes are binary incompatible "by design", the whole point of the PR is to no longer extend the things that were extended before. I wonder what binary compatibility guarantees we have promised for this project though - we tend to be a bit less strict in projects that are only used in test scope. Perhaps someone more intimately familiar with the project can say something about that? |
Any updates on this ? I need it ! |
I've associated the commit email with my account - so that's hopefully good to go I think that just leaves the binary compatibility issue from MIMA, and guidance on what we want to do there |
this allows implementing specs to use either sync or async suites