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

Don't exit with number of failures because Java will mod 256 the exitcode so System.exit(512) == System.exit(256) == System.exit(0) #4106

Merged
merged 1 commit into from Dec 2, 2016

Conversation

Projects
None yet
5 participants
@cheister
Copy link
Contributor

cheister commented Dec 1, 2016

Problem

If you have a test with 256 failures it will return success

Solution

Don't System.exit(failureCount)

@cheister

This comment has been minimized.

Copy link
Contributor

cheister commented Dec 1, 2016

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Dec 1, 2016

Thanks for this. Could you add a regression test?

@cheister

This comment has been minimized.

Copy link
Contributor

cheister commented Dec 1, 2016

I can. I wasn't sure if we wanted a test with 256 failures just to test this case?

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Dec 1, 2016

You could generate a file with 256 test cases in an integration test, or use a parameterized runner with an array.

@mateor

This comment has been minimized.

Copy link
Member

mateor commented Dec 1, 2016

This at least mimics Unix (ETA: POSIX? my shell, at any rate) behavior.

mateo:1 mateo$ $(exit 2000)
mateo:1 mateo$ echo $?
208
mateo:1 mateo$ echo $((2000 % 256))
208
Chris Heisterkamp
Don't exit with number of failures because Java will mod 256 the exit…
… code so System.exit(512) == System.exit(256) == System.exit(0)

@cheister cheister force-pushed the cheister:fix-junit-exit-code branch from 1105c28 to 5723bd6 Dec 1, 2016

@cheister

This comment has been minimized.

Copy link
Contributor

cheister commented Dec 1, 2016

The test has been added, it can't be run in CI until we merge the change and update the junit_runner jar with the change.

@stuhood

stuhood approved these changes Dec 2, 2016

Copy link
Member

stuhood left a comment

Thanks Chris! And thanks for the test.

@stuhood stuhood merged commit 5423eee into pantsbuild:master Dec 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cheister cheister deleted the cheister:fix-junit-exit-code branch Dec 2, 2016

stuhood added a commit that referenced this pull request Dec 3, 2016

Update junit-runner to 1.0.17 (#4113)
### Problem

Need to update junit-runner to pick change in #4106

### Solution

Update junit-runner and uncomment test showing the new change is working.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Don't exit the JUnitRunner with number of failures because Java will …
…mod the exit code. (pantsbuild#4106)

### Problem

If you have a test with 256 failures it will return success

### Solution

Don't System.exit(failureCount)

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Update junit-runner to 1.0.17 (pantsbuild#4113)
### Problem

Need to update junit-runner to pick change in pantsbuild#4106

### Solution

Update junit-runner and uncomment test showing the new change is working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment