Closed
Bug 1261412
Opened 8 years ago
Closed 8 years ago
Enable running specific file or dir with mach python-test, outside of PYTHON_UNIT_TESTS
Categories
(Firefox Build System :: Mach Core, enhancement)
Firefox Build System
Mach Core
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: impossibus, Assigned: impossibus)
References
Details
Attachments
(3 files)
[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•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43871/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43871/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43873/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43873/
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mjzffr
Assignee | ||
Comment 5•8 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•8 years ago
|
Attachment #8737282 -
Flags: review?(ted)
Attachment #8737283 -
Flags: review?(ted)
Attachment #8737284 -
Flags: review?(ted)
Comment 6•8 years ago
|
||
(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•8 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.
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
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•8 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 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Assignee | ||
Comment 21•8 years ago
|
||
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/
Assignee | ||
Comment 22•8 years ago
|
||
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 23•8 years ago
|
||
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 24•8 years ago
|
||
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•8 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
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•