gradle / gradle

Adaptable, fast automation for all
https://gradle.org
Apache License 2.0
16.95k stars 4.75k forks source link

Detect System.exit() being called as part of (functional) Java tests #3610

Open sschuberth opened 6 years ago

sschuberth commented 6 years ago

Expected Behavior

If a functional Java test case contains a code path that executes System.exit() not only the program under test is terminated, but also the test framework itself. This may result in subtle errors like remaining test cases not being executed, shadowing potential test breakages.

It was wondering if Gradle could do something smart to prevent such errors, e.g. by comparing the number of executed test cases to the number of test cases discovered via reflection.

Current Behavior

Currently, if a test case containing System.exit() is executed, a call to ./gradlew test on Linux will silently skip any remaining tests. See how here MainTest is not followed by MavenTest, and also there's no indication whether MainTest passed or not. Still Gradle proceeds and in the ends reports BUILD SUCCESSFUL.

For the correct output on Linux see here.

On Windows, the symptoms are slightly different: See how here Gradle reports about an unexpected MessageIOException but again succeeds in the end.

For the correct output on Windows see here.

Context

Gradle not failing despite the unexpected exception being throw has resulted in bugs slipping into the mainline as remaining test cases were not executed.

Your Environment

Gradle 4.3 (via wrapper), Kotlin 1.1.60, kotlintest, targeting JVM 1.8, both Windows 10 and Ubuntu 17.10, 64-bit each.

lipnitsk commented 6 years ago

Just ran into this in my project and spent a few quality hours chasing down the problem. It would certainly be nice if gradle didn't report a successful run in case System.exit() was called by a test prematurely.

lipnitsk commented 6 years ago

@blindpirate this sounds more like a bug than a feature, don't you think?

sschuberth commented 6 years ago

It's certainly not a bug. As long as there's only one JVM instance that both executes the test runner and the tests themselves, that's just how System.exit() is specified to work. However, it just occurred to me that the problem maybe can be worked around by forking a separate JVM that executes the code to test only.

lipnitsk commented 6 years ago

I would argue that tests that expect the code under test to execute System.exit() should use a rule for that. How is it expected that the rest of the test suite is affected by a single test without gradle reporting any error or message?

sschuberth commented 6 years ago

I would argue that tests that expect the code under test to execute System.exit()

That's not my use-case. My use-case is that you write a functional test, and you're not aware in the beginning (or forgot about) that the code path the functional tests triggers is going to execute System.exit(). Even such an oversight should not bring test whole test suite down.

lipnitsk commented 6 years ago

Agreed, I think we are on the same page :)

jduan commented 5 years ago

Any updates/comments from the Gradle core team?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you're interested in how we try to keep the backlog in a healthy state, please read our blog post on how we refine our backlog. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

jduan commented 4 years ago

I feel this is a pretty big problem. It's a ticking bomb. A new System.exit() can be introduced any time (in code or tests) and CI would silently start bypassing a lot of tests. For a large organization that has thousands of tests, unless they have monitoring in place (eg: monitor the trend of executed tests), this can easily slip under the radar for a while before anyone notices the problem.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you're interested in how we try to keep the backlog in a healthy state, please read our blog post on how we refine our backlog. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

stale[bot] commented 2 years ago

This issue has been automatically closed due to inactivity. If you can reproduce this on a recent version of Gradle or if you have a good use case for this feature, please feel free to to let know so we can reopen the issue. Please try to provide steps to reproduce, a quick explanation of your use case or a high-quality pull request.

alwa commented 1 year ago

I'm still observing this issue when running a big project with many (mostly Robolectric) tests.

Gradle: 8.1.1 JDK: 17