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

DefaultLauncher should catch exceptions thrown by test engines #750

Closed
1 task done
jlink opened this issue Mar 26, 2017 · 7 comments
Closed
1 task done

DefaultLauncher should catch exceptions thrown by test engines #750

jlink opened this issue Mar 26, 2017 · 7 comments

Comments

@jlink
Copy link
Contributor

jlink commented Mar 26, 2017

Overview

If a test engine throws an exception during discovery (see https://github.com/junit-team/junit5/blob/master/junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncher.java#L115) other engines won't be queried any more.

I think this is a bug because one test engine can thereby prevent others from working.

Deliverables

  • DefaultLauncher should catch and report exceptions from engines and proceed with other engines.
    • This applies to both the discovery phase and execution phase.

Additional Proposals

  • DefaultLauncher could even collect discovery results concurrently.
@sbrannen
Copy link
Member

Thanks for raising the issue.

I totally agree!

The same applies to the execution phase as well.

@sbrannen
Copy link
Member

DefaultLauncher could even collect discovery results concurrently.

It could do that, but I consider that a separate topic. I have therefore updated this issue's description to reflect the deliverables.

Feel free to create a new issue to propose concurrent test discovery across registered engines.

@sbrannen
Copy link
Member

Slated for M5 since we are trying to get M4 out the door as soon as possible.

@sormuras sormuras self-assigned this Apr 10, 2017
sormuras added a commit that referenced this issue Apr 10, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 11, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 11, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 13, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 13, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 13, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 15, 2017
State: work-in-progress

Addresses: #750
sormuras added a commit that referenced this issue Apr 15, 2017
State: work-in-progress

Addresses: #750
@sormuras
Copy link
Member

With #792 merged, an erroneous engine implementation doesn't any longer break the test plan execution.

We should decide if a "simple warning" is enough for the end-user to take note of the failed engine. Perhaps the execution summary needs an update, like:

Test run finished after 3091 ms
[         2 engines found         ]
[         1 engines successful    ]
[         1 engines aborted       ]
[         0 containers skipped    ]
[        70 containers started    ]
[         0 containers aborted    ]
[        70 containers successful ]
[         0 containers failed     ]
[       602 tests found           ]
[         0 tests skipped         ]
[       602 tests started         ]
[         1 tests aborted         ]
[       601 tests successful      ]
[         0 tests failed          ]

Updated issue description accordingly.

@sbrannen
Copy link
Member

We should decide if a "simple warning" is enough for the end-user to take note of the failed engine. Perhaps the execution summary needs an update, like:

I think we should go ahead and close this issue and address the topic of exception handling vs. logging in conjunction with #242 (thereby broadening the scope of #242 to cover the Platform as well).

@marcphilipp
Copy link
Member

👍

@sbrannen
Copy link
Member

I have moved the second deliverable to #242 and am therefore closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants