Closed Bug 1494090 Opened Last year Closed 2 months ago

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

Categories

(Testing :: Python Test, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: ahal, Assigned: anmol.agarwal001, Mentored)

Details

Attachments

(1 file)

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).
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.
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.
(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.
Severity: normal → enhancement

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.

Flags: needinfo?(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.

Flags: needinfo?(ahal)

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.

Flags: needinfo?(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.

Flags: needinfo?(ahal)

Hey ahal,

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

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

Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4167cec91574
Added -x option to |mach python-test| r=ahal

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.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Assignee: nobody → anmol.agarwal001
You need to log in before you can comment on or make changes to this bug.