1494090 - Add wrapper around pytest's '-x' flag to |mach python-test|

Status: RESOLVED FIXED

(Testing :: Python Test, enhancement)

Description

[Andrew Halberstadt [:ahal]]

Passing in -x to pytest aborts after the first failure. This is *really* useful when you have multiple failures in a single test file and want to debug them one at a time.

It's currently possible to do this with:
$ PYTEST_ADDOPTS=-x ./mach python-test ...

But we should make it an official argument. Since every test file is run in a multiprocess, we would need some extra logic in python/mach_commands.py to abort remaining test files when an error is detected (though even if we dumbly pass the argument down, it would still be valuable).

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

According to |pytest --help| the |-x| option is essentially a synonym for |--maxfail=1|:

>  -x, --exitfirst       exit instantly on first error or failed test.
>  --maxfail=num         exit after first num failures or errors.

If we're going to add an argument to |mach python-test| then do we want to also add the ability to exit after more than one failure?

I'd personally prefer a way to pass any command line options unrecognised by |mach python-test| directly to pytest. This was also suggested in bug 1433941, and I've seen other instances where it would be useful to add pytest options conveniently via mach.

Comment 2

[Andrew Halberstadt [:ahal]]

True, I'd be happy with passing all command line flags too. But this particular one will require a bit of extra logic in the mach command since we run one pytest command per test file. Maybe we can do the argument pass through as a pre-cursor commit within this bug.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> True, I'd be happy with passing all command line flags too. But this
> particular one will require a bit of extra logic in the mach command since
> we run one pytest command per test file. Maybe we can do the argument pass
> through as a pre-cursor commit within this bug.

Longer term I'd like to see us moving closer to a pytest plugin model, where we could hook our test discovery into pytest itself and avoid invoking pytest for each test file. This will improve the test logs, and would allow us to take advantage of more pytest command line options and plugins.

For now, let's keep this simple as you originally suggested and just add a `-x` argument to `mach python-test` that aborts on the first failure.

Comment 4

[Anmol Agarwal]

Hey ahal,

As far as I have understood by reading the bug summary and some of the code in python/mach_commands.py, what I got is that there are some tests which are run in parallel (concurrently) and some sequentially. Now the requirement is to add an option -x, which will stop the test as soon as there is a failure. I have got confusion among the following two:

1. Here the first failure is considered as a test object/file. If yes, then let's say there are 5 sequential test objects/files and the first failure is at object 3, then other 2 won't be processed. But, then how will this be handled for parallel running tests?
2. Here the first failure is considered as a test function inside a test object/file. If yes, then as per the above example all 5 test objects will be processed. But, if there are 6 test functions in test object 1 and test function 2 fails, then the remaining 4 shouldn't be processed.

Please clarify.

Comment 5

[Andrew Halberstadt [:ahal]]

It's the second one, a failure is a test function within a test file. Getting this part working is easy, we just need to forward -x to the pytest command line:
https://searchfox.org/mozilla-central/source/config/mozunit/mozunit/mozunit.py#248

Maybe we can even just set the PYTEST_ADDOPTS environment variable.

The tricky bit is going to be how do we stop all of the other threads when one of the files fails. There are two possibilities here:

1. If passing -x, just run all tests in sequence.
2. If a test file returns non-zero, then we raise and catch an exception and then try and stop the threads like this:
   https://searchfox.org/mozilla-central/source/python/mach_commands.py#181

I'd vote we at least attempt #2 and see if it'll work. If not we can just do the sequential thing.

Comment 6

[Anmol Agarwal]

Hey ahal,

If it's the second one, then why do we need to stop the other threads if one of it fails. What I understand is, let's say there are 3 sequential jobs (Job id 1-3) and 3 parallel jobs (Job id 4-6). Here I am considering that one job is associated to one test file as per your first comment.

Sequential Jobs:

• Job 1 has 5 cases and all of them were a success
• Job 2 has 4 cases and failed at case 2
• Job 3 has 5 cases and failed at case 3

Parallel Jobs:

• Job 4 has 5 cases and failed at case 2
• Job 5 has 4 cases and all were success
• Job 6 has 4 cases and failed at case 3

Then according to me, the output will be something like (if we pass -x option):

Job 1: All passed cases
Job 2: Case 1 passed, Case 2 failed and Case 3, 4 skipped
Job 3: Case 1, 2 passed, Case 3 failed and Case 4, 5 skipped
Job 4: Case 1 passed, Case 2 failed and Case 3, 4, 5 skipped
Job 5: All cases passed
Job 6: Case 1, 2 passed, Case 3 failed and Case 4 skipped

The difference would be that Jobs 1, 2 and 3 would have generated their outputs by running sequentially. Jobs 4, 5, 6 would have generated their outputs by running in parallel.

Please correct if I am wrong. Can you please use the same example to explain if wrong.

Comment 7

[Andrew Halberstadt [:ahal]]

The purpose of -x is to debug test failures rapidly, e.g maybe there are multiple failures and you want to focus on one at a time. So you pass -x and your run exits as soon as it hits a failure. This A) ensures the failure is at the bottom of your terminal, and B) means you don't need to wait for the rest of your tests to finish.

However, typing this out I realized something. If we continue to run tests in parallel then the order of execution of tests isn't guaranteed. This is undesirable because it makes debugging a specific failure difficult. So I've changed my mind and think we should go with option 1. When passing in -x, all tests should be run in sequence.

Comment 8

[Anmol Agarwal]

Hey ahal,

I just raised a revison review request. Please see if it meets the requirements https://phabricator.services.mozilla.com/D47321

Comment 9

[Anmol Agarwal]

Attachment: Bug 1494090 - Added -x option to |mach python-test|

Bug 1494090 - Added -x option to |mach python-test|

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4167cec91574
Added -x option to |mach python-test| r=ahal

Comment 11

[Anmol Agarwal]

Hey ahal,

Please assign this bug to me. Moreover, as per your comments on the revision, it looks this this has been resolved. If yes, please change the status too.

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4167cec91574