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

Catch exceptions raised in parallelize_setup and ensure tests fail when they occur #36018

Merged
merged 1 commit into from Apr 18, 2019

Conversation

aricwalker
Copy link
Contributor

Summary

The changes here ensure that we catch any StandardExceptions that occur during the after_fork call (blocks passed into parallelize_setup) and record those exceptions with the test results. This is work towards resolving #35835.

This results in marking all of the tests as failed to bring attention to the issue & ensure it is addressed before proceeding.

/cc #35835
/cc @eileencodes

@aricwalker
Copy link
Contributor Author

In testing this against the example app noted in #35835, the results are exiting properly now (with 1):

# Running:

E

Error:
TestClass#test_something:
RuntimeError: Expected to fail `rails test`
    test/models/model_test.rb:3:in `block in <class:TestClass>'

Failure:
TestClass#test_something:
Expected false to be truthy.


rails test test/models/model_test.rb:6



Finished in 0.091489s, 10.9303 runs/s, 10.9303 assertions/s.
1 runs, 1 assertions, 0 failures, 1 errors, 0 skips

$ echo $?
1

Instead of with 0:

Finished in 0.222651s, 0.0000 runs/s, 0.0000 assertions/s.
0 runs, 0 assertions, 0 failures, 0 errors, 0 skip

$ echo $?
0

@eileencodes eileencodes added this to the 6.0.0 milestone Apr 18, 2019
@eileencodes eileencodes self-assigned this Apr 18, 2019
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit from me, but otherwise good. I’ll let Eileen take it from here.

@@ -81,6 +83,8 @@ def start
reporter = job[2]
result = Minitest.run_one_method(klass, method)

add_setup_exception(result, setup_exception) if setup_exception.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t think we need present? here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I don't believe we do. Removed in ccc2ed63c7

@kaspth
Copy link
Contributor

kaspth commented Apr 18, 2019

Cool, then your commits just need to be squashed. There’s no need for a changelog entry since parallel testing isn’t out yet.

Resolves rails#35835

If an exception occurs during `parallelize_setup` make sure to catch that exception and apply it to the result of each successive test run.  This results in marking all of the tests as failed to bring attention to the issue & ensure it is addressed before proceeding.
@aricwalker
Copy link
Contributor Author

Just squashed the commits.

There’s no need for a changelog entry since parallel testing isn’t out yet.

👍

@eileencodes eileencodes merged commit 8ac4d15 into rails:master Apr 18, 2019
@aricwalker aricwalker deleted the parallel-setup-exception branch April 18, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants