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

Introduce UnrecoverableThrowable abstraction for exception that should terminate test plan #3037

Open
edudar opened this issue Sep 17, 2022 · 8 comments

Comments

@edudar
Copy link

edudar commented Sep 17, 2022

Only one error terminates the test plan: OutOfMemoryError. Sometimes it might be beneficial to have more control over this. In my case, I'm bringing up test containers before the test suite starts. I implemented this using TestExecutionListener. I start containers in testPlanExecutionStarted and stop them in testPlanExecutionFinished. It happens from time to time that a container does not start. In this case, I want to terminate the test plan because it does not make sense to run any tests when some required container is down.

It'd be good to introduce a class or, better, an interface like UnrecoverableThrowable and check if throwable is an instance of OutOfMemory || UnrecoverableThrowable. Then users could create custom exceptions that implement said interface and terminate the test plan.

If this idea sounds, I can create a PR.

@marcphilipp
Copy link
Member

I think a more flexible option would be to introduce a configuration parameter for configuring a comma-separated list of fully-qualified class or interface names for such exceptions. WDYT?

@edudar
Copy link
Author

edudar commented Sep 18, 2022

Like this https://junit.org/junit5/docs/current/user-guide/#running-tests-config-params? Never had a chance to need them but I think that'll be ok.

junit.jupiter.exceptions.unrecoverable = \
    abc.xyz.Exceptions$OopsyException

Assuming I can put junit-platform.properties into resources in a library so that it can be used by a bunch of independent services.

@sormuras
Copy link
Member

sormuras commented Sep 18, 2022

Perhaps, we want to support both at the same time? Having an Unrecoverable tagging interface users my extend in their code base and a configurable list of types for exception types they don't control, seem like good thing to support.

🤔 ... if there was a PleaseStopException extends RuntimeException implements Unrecoverable {} built-in into Jupiter, would that also be an alternate solution for the underlying feature request described in:

@sbrannen
Copy link
Member

Assuming I can put junit-platform.properties into resources in a library so that it can be used by a bunch of independent services.

I don't think that would work. IIRC, JUnit only looks for a single junit-platform.properties file in the root of the classpath.

I think we might have an open (or closed due to inactivity) issue related to specifying a different location for the file or potentially supporting multiple files.

If we don't introduce support for multiple files, a feature like this would have to addressed differently -- perhaps via the ServiceLoader mechanism, although that would be a little odd for a "service" to only provide a list of types.

@edudar
Copy link
Author

edudar commented Sep 19, 2022

Thinking more about multiple files, they may cause more problems than they solve. Considering that there are preferences like something.enabled=true with multiple files it would open a can of worms with conflicting preferences coming from different JARs. I'd stick to an interface or some annotation-based solution (that would solve my case) and a single properties file (as it's now) for greater flexibility / 3rd-party exceptions.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Sep 21, 2022

Might the original use case not be better supported by #2816?

@edudar
Copy link
Author

edudar commented Sep 21, 2022

@mpkorstanje, unless I'm missing something about that feature, I don't see how. In this case, I want to stop the test plan because something went wrong during initialization. Not sure how an option to load closable resources into the context would help with that.

And if it's about starting/stopping containers, then I'd guess it could be done using that closable resource abstraction (except it's not there yet), but if that resource wouldn't load, I'd still need a way to stop the test plan somehow.

@mpkorstanje
Copy link
Contributor

Your need to stop the test plan with an exception is a solution to a more general need to be able to create a resource prior to the execution of the test plan. Using a listener may not be the right abstraction.

I'd still need a way to stop the test plan somehow.

It's my assumption that the test plan would fail before it got executed if the resources could not be created.

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

5 participants