1261412 - Enable running specific file or dir with mach python-test, outside of PYTHON_UNIT_TESTS

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

[I'm splitting this work off from Bug 1227367 (Marionette Python harness tests) since it's not essential for deploying the harness tests and it affects other projects.]

We'd like to run our pytest-style tests via mach. Originally, we had settled on running these tests with ./mach python-test. However, since Bug 1255479, ./mach pytest-test is designed to run tests that belong to PYTHON_UNIT_TESTS. In order to fit pytest-style tests in that category, we have to better integrate pytest into existing automation (vendor-in pytest so it can be imported during make check (?), fix pytest output to match automation expectations) and I think it's too early for any of that, and may not make sense at all at this point.

The other option is to have a separate ./mach pytest command; it's the option I currently favour.

I will attach my previous patches for both approaches (./mach python-test and ./mach pytest) for the sake of discussion.

Comment 1

[(not currently active) Ted Mielczarek]

I'm not particularly excited about having two separate "Run Python tests" commands, honestly. I don't have any particular attachment to the commands' current expectations of output format, I'd be happy to see that fixed so it works for other styles of tests. (I also wish it was all using structured logging too, honestly.) I assume the output check was primarily to sanity check that the thing you're running is actually a test and not just some random Python file.

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps

(WIP; breaks the build)

This uses the same approach as with mozunit to select pytest as the test runner.

Add Marionette harness tests to PYTHON_UNIT_TESTS

I also create a pytest plugin that modifies the pytest output to include
a string expected by the ./mach python-test command. I wrote it as a named
plugin, rather than just including it in conftest.py, so that it can be
imported into other test suites in the future.

Review commit: https://reviewboard.mozilla.org/r/43869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43869/

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: MozReview Request: Bug 1261412 - Report when mach python-test collects no tests; r?gps

Review commit: https://reviewboard.mozilla.org/r/43871/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43871/

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

Review commit: https://reviewboard.mozilla.org/r/43873/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43873/

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> I'm not particularly excited about having two separate "Run Python tests"
> commands, honestly. I don't have any particular attachment to the commands'
> current expectations of output format, I'd be happy to see that fixed so it
> works for other styles of tests. (I also wish it was all using structured
> logging too, honestly.) I assume the output check was primarily to sanity
> check that the thing you're running is actually a test and not just some
> random Python file.

The output check isn't a problem; I more concerned with having to include pytest-style tests in PYTHON_UNIT_TESTS, because this has implications on the build.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #5)
> The output check isn't a problem; I more concerned with having to include
> pytest-style tests in PYTHON_UNIT_TESTS, because this has implications on
> the build.

What implications are you worried about? I'm not sure why we'd want to have tests in-tree that we didn't intend to run in automation.

Comment 7

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> (In reply to Maja Frydrychowicz (:maja_zf) from comment #5)
> > The output check isn't a problem; I more concerned with having to include
> > pytest-style tests in PYTHON_UNIT_TESTS, because this has implications on
> > the build.
> 
> What implications are you worried about? I'm not sure why we'd want to have
> tests in-tree that we didn't intend to run in automation.

We definitely want to run them in automation, but it's not necessary to run them as part of the build process. I've encountered two problems:
* Originally just wanted to call the mach command to run these harness tests in TaskCluster. Due to Bug 1255479, I must run ./mach configure first, which my TC image doesn't have the prerequisites for. (The whole point of that TC task is to avoid depending on the build or its prerequisites.)
* So I won't use the mach command in TaskCluster, that's fine, but in any case, to get ./mach python-tests working with pytest, I also need to address this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87eff7539444&selectedJob=18843098 -- which leads me to wonder whether it makes sense to to make these adjustments given that the harness tests have nothing to do with the build. It feels like trying to cram something where it doesn't belong, while possibly breaking more stuff. That's why I'm back to favouring the "separate mach command" solution, but I totally understand your hesitation.

Comment 8

[James Graham [:jgraham]]

ato's work to integrate pytest with web-platform-tests means that it will already be in-tree, and integrating it with structured logging will be trivial (wpt does it, but not in an entirely generic way).

Out of interest is the mach command problem related to naming the command after implementation details rather than after the scope of what's being tested? I have no idea what kind of thing |mach pytest| or |mach python-test| will test, just that they use python.

Comment 9

[Andreas Tolfsen ❲:ato❳]

My work on WPT means it will be in tree under /testing/web-platform-tests, and if this is a fine place for mach to pick it up from that’s good.  But because other browser vendors rely on WPT we unable to move it out of wpt-tools, however.

It would be great it someone would write a structured log plugin for pytest.  Inspired by davehunt’s pytest-html plugin, the essence is captured in wptrunner.executors.pytestrunner on how to intercept test results: https://github.com/w3c/wptrunner/blob/master/wptrunner/executors/pytestrunner/runner.py#L63

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

14:01 <maja_zf> ted: re mach python-test -- what about providing a flag to just run a specific test/dir/manifest (more or less the way it did before, without looking at PYTHON_UNIT_TESTS?)
14:03 <•ted> maja_zf: just passing a file/manifest to `mach python-test` and having it run that seems totally reasonble

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8737282 [details]
MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps

https://reviewboard.mozilla.org/r/43869/#review41265

This seems reasonable.

Withholding r+ because the commit message says it is a WIP.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8737283 [details]
MozReview Request: Bug 1261412 - Report when mach python-test collects no tests; r?gps

https://reviewboard.mozilla.org/r/43871/#review41267

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

https://reviewboard.mozilla.org/r/43873/#review41269

Pretty sure you don't want me to review this since it appears the consensus from the bug is that `mach python-test` will accept a manifest to a pytest.

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737283 [details]
MozReview Request: Bug 1261412 - Report when mach python-test collects no tests; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43871/diff/1-2/

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737282 [details]
MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43869/diff/1-2/

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43873/diff/1-2/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8737282 [details]
MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps

https://reviewboard.mozilla.org/r/43869/#review43469

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

https://reviewboard.mozilla.org/r/43873/#review43471

::: python/mach_commands.py:104
(Diff revision 2)
> +                elif os.path.isdir(t):
> +                    files += glob.glob(mozpath.join(t, 'test*.py'))
> +                    files += glob.glob(mozpath.join(t, 'unit*.py'))

This feels like it should descend recursively, which glob() doesn't do, unfortunately.

You should probably use `os.walk()` here. Alternatively, you could import `unittest` and use its functions for finding tests. I'm not sure what those functions are - you'd have to look at the source code.

Feel free to punt on this as a follow-up.

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737283 [details]
MozReview Request: Bug 1261412 - Report when mach python-test collects no tests; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43871/diff/2-3/

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737282 [details]
MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43869/diff/2-3/

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43873/diff/2-3/

Comment 22

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

Small update with os.walk. Flagging manually for re-review, since MozReview keeps the r+.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8737284 [details]
MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps

https://reviewboard.mozilla.org/r/43873/#review43767

Cool.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f3009caf1ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/b79810219014
https://hg.mozilla.org/integration/mozilla-inbound/rev/d06c65a39449

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f3009caf1ef
https://hg.mozilla.org/mozilla-central/rev/b79810219014
https://hg.mozilla.org/mozilla-central/rev/d06c65a39449