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

RESOLVED FIXED in Firefox 48

Status

Firefox Build System
Mach Core
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: maja_zf, Assigned: maja_zf)

Tracking

unspecified
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

[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.
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8737282 [details]
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/
Attachment #8737282 - Flags: review?(gps)
Attachment #8737283 - Flags: review?(gps)
Attachment #8737284 - Flags: review?(gps)
(Assignee)

Comment 3

2 years ago
Created attachment 8737283 [details]
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/
(Assignee)

Comment 4

2 years ago
Created attachment 8737284 [details]
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/
(Assignee)

Updated

2 years ago
Assignee: nobody → mjzffr
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8737282 - Flags: review?(ted)
Attachment #8737283 - Flags: review?(ted)
Attachment #8737284 - Flags: review?(ted)
(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.
(Assignee)

Comment 7

2 years ago
(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.
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.
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
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
Summary: mach command for running pytest-style tests → Enable running specific file or manifest with mach python-test, outside of PYTHON_UNIT_TESTS
(Assignee)

Updated

2 years ago
No longer depends on: 1227367
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.
Attachment #8737282 - Flags: review?(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
Attachment #8737283 - Flags: review?(gps) → review+
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.
Attachment #8737284 - Flags: review?(gps)
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/
Attachment #8737283 - Attachment description: MozReview Request: Bug 1261412 - Report when mach python-tests collects no tests; r?gps → MozReview Request: Bug 1261412 - Report when mach python-test collects no tests; r?gps
Attachment #8737282 - Attachment description: MozReview Request: Bug 1261412 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps → MozReview Request: Bug 1261412 - Relax test output requirement in mach python-test; r?gps
Attachment #8737284 - Attachment description: MozReview Request: Bug 1261412 - Add pytest command to mach; r?gps → MozReview Request: Bug 1261412 - Add mach python-test option to collect tests based on path alone; r?gps
Attachment #8737283 - Flags: review?(ted)
Attachment #8737282 - Flags: review?(ted) → review?(gps)
Attachment #8737284 - Flags: review?(ted) → review?(gps)
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 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/
(Assignee)

Updated

2 years ago
Summary: Enable running specific file or manifest with mach python-test, outside of PYTHON_UNIT_TESTS → Enable running specific file or dir with mach python-test, outside of PYTHON_UNIT_TESTS
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
Attachment #8737282 - Flags: review?(gps) → review+
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.
Attachment #8737284 - Flags: review?(gps) → review+
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 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 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 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+.
Attachment #8737284 - Flags: review+ → review?(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.
Attachment #8737284 - Flags: review?(gps) → review+

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f3009caf1ef
https://hg.mozilla.org/mozilla-central/rev/b79810219014
https://hg.mozilla.org/mozilla-central/rev/d06c65a39449
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.