Do not run rake test tasks with at_exit

[codez]

For rake test tasks, call Minitest.run explicitly instead of using the at_exit hook. This executes test tasks at their specified position and not only at the end of the rake run.

E.g.

rake test:models other:task test:controllers

This first runs the model tests, then other:task and finally the controller tests. Before this change, other:task was run first, then the model and controller tests were run in one go.

If a test task fails, rake exits with exit code 1 (same as before) without running the remaining tasks.

Fixes #17708.

[arthurnn]

This is a good start, me and @senny were discussing this.
One thing tho, from your implementation, is that I dont think we need to use a env var (RUN_TESTS_AT_EXIT) to control this, we could not autorun when the rails runner is loaded, you can check the global var $rails_test_runner to see if the runner is loaded or not.

[codez]

Relying on $rails_test_runner would also mean that rails t runs are not using the at_exit hook. I didn't investigate this possibility, but then we'd have a uniform behaviour in both cases. Should make sense.

[codez]

I adjusted the commit to use $rails_test_runner instead of the env var.

[arthurnn]

As far as I can tell, we are only matching if those output exist, and not the order of them.
We should match the order too, to test that at_exit from Minitest is not running.

[codez]

The order is matched in test_run_test_task_before_others. This test asserts that the dummy task is not executed when the previous test task fails. As only "FooTest" appears in the output, there is no order to test.

[floehopper]

Interesting, I'd somehow missed this PR. I did some investigation into something similar in this Minitest issue.

Since this commit, because of the "global" random ordering, typically some integration tests run before some unit tests. While I understand the desire to run all the tests in a single process (to avoid Rails environment load time), the fact that some integration tests are now run before some unit tests seems wrong to me.

In my opinion, unit tests should give the fastest and most-specific feedback and should all be run first, even if they aren't run in a separate process. Also if any unit test fails, no integration tests should be run.

Would you be open to making such a change and would it make sense to incorporate it into this PR or should it be a separate PR?

[senny]

@floehopper With Rails 5 we will be moving away from rake to run our tests (See this PR) for more details. Nonetheless, in my opinion a bin/rails test will continue to require all the test files and run them randomly. However the new API will allow you to easily add a script / task to get the desired behavior you want:

Minitest.run(["test/models", "test/controllers", "test/helpers"])
Minitest.run(["test/functional"])
Minitest.run(["test/integration"])

I don't think this should be the default behavior though.

/cc @kaspth @arthurnn

[kaspth]

Yes, I agree with @senny. And that code is how you'd do it 👍

[floehopper]

@senny, @kaspth: Can you explain why you think it's better to run all the tests in one group?

[kaspth]

The benefit just doesn't sound that appealing. It's far simpler for us to just let Minitest randomize the tests 😄

[floehopper]

Unit tests should run much quicker than integration tests and when they fail they give a much better indication of where the problem lies compared to a failing integration test. I think there is a significant advantage in running all the unit tests before any of the integration tests and I don't think it would make the code much more complicated to do so.

However, I recognise that this suggestion is separate from the purpose of the changes in this PR, so I won't add any more comments here.

[kaspth]

Fixed in #19571. Thanks for your work ❤️