Closed Bug 1227367 Opened 9 years ago Closed 9 years ago

Add harness unit tests to marionette-client

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed, firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

Details

(Keywords: pi-marionette-harness-tests, pi-marionette-runner)

Attachments

(6 files, 3 obsolete files)

MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin
58 bytes, text/x-review-board-request
dustin
: review+
Details
MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund
58 bytes, text/x-review-board-request
jlund
: review+
Details
:automatedtester mentioned that .t tests might be useful - https://pypi.python.org/pypi/cram
I've started writing the unit tests using pytest. Also investigating how these tests should be run and reported. The two most-likely options are: 1. Define a new TaskCluster task. 2. Run the tests as part of the build (as part of 'make check'), similarly to what's done for mozbase tests. There's no support for pytest right now, so this would involve vendoring pytest in m-c. The relevant moz.build variable might be PYTHON_UNIT_TESTS if I can get that to work with pytest. Otherwise, maybe I have to modify how PYTHON_UNIT_TESTS is processed? I'm leaning toward TaskCluster, although I need to investigate some more. Re Option 2, there are plans to move tests out of the build anyway (:ahal), so I'm not sure it's worth the trouble adding in pytest support.
If we go with Taskcluster I wonder if each of the unit test will run in its own task or if we want to combine known unit tests into a single task. In regards of setup time it might make sense to combine them.
Assignee: nobody → mjzffr
TaskCluster it is. WIP for the TC task: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edbb2064416d&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3 I used a task-image based on 'desktop-build', but I suspect that I won't be able to pip install pytest in that environment. Might need 'desktop-test' instead. I will look into this and ask for feedback on the next iteration.
(In reply to Maja Frydrychowicz (:maja_zf) from comment #4) > I used a task-image based on 'desktop-build', but I suspect that I won't be > able to pip install pytest in that environment. Might need 'desktop-test' > instead. I will look into this and ask for feedback on the next iteration. FYI what has been shown in the past is that you should not use a build machine to run tests on. Not sure what specifically will be different with TC but on Buildbot those machines are kinda differently setup. Thanks for posting the link to the WIP. It looks pretty straight forward to get something setup.
Latest WIP for the TC task is here, now with mach! https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bffe92532c4&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=16223476 dustin, ahal -- could you take a look and give me some feedback on the following? * https://hg.mozilla.org/try/rev/6b7479c4aa30 * https://hg.mozilla.org/try/rev/6f2f09fe0877 Many details are missing, but I just wanted to sketch out how these harness tests would run with TaskCluster. Is it alright to be using mach? For the image, any objections to using desktop-build in this case? In the mean time, I'll go back to writing the actual harness tests.
Flags: needinfo?(dustin)
Flags: needinfo?(ahalberstadt)
(In reply to Maja Frydrychowicz (:maja_zf) from comment #6) > dustin, ahal -- could you take a look and give me some feedback on the > following? > * https://hg.mozilla.org/try/rev/6b7479c4aa30 > * https://hg.mozilla.org/try/rev/6f2f09fe0877 Also, is it possible to move the `tc-vcs checkout` command to the base yml file (harness_test.yml), while keeping `./mach marionette-harness-test` in harness_marionette.yml? I'm not familiar with the syntax, but when I've tried to have commands in both the base and the child, they get concatenated in a way that fails to run because of how bash is called. For example, I'd like harness_marionette.yml to contain just: > command: > - ./mach marionette-harness-test ...and that should run right after the source has been checked-out with a command in harness_test.yml.
You might consider putting harness tests in a different subdirectory, maybe tasks/harness Using desktop-build is fine as long as the tests expect to run in the same environment where builds take place. If you're using mach, especially, then I think it is a good image to use. I dislike the idea of chaining shell commands together in the .yml files. Instead, it's better to put the logic into shell scripts and control them via environment variables. You'll note that the build process that it runs `bin/build.sh` which is burned into the image, but only does the clone and runs `testing/taskcluster/scripts/builder/build-linux.sh` out of the resulting clone. The latter can then be changed easily in a try push without re-creating the docker image. That build-linux.sh script then takes a bunch of env vars as parameters to determine how to run mozharness.
Flags: needinfo?(dustin)
I'm generally not a huge fan of running mach from automation, though other people on our team will disagree with me :). Is this bypassing mozharness? If so, I guess using mach as an entry point is ok.. though most test jobs use mozharness. If you want to make this job consistent with everything else though, you'll probably want to either make a new mozharness script, or try to re-use an existing one. If you go this route you should inherit from fx_test_generic.yml, which will run this script to invoke mozharness: https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/tester/test-linux.sh Notice, you'll need to set some environment variables, like MOZHARNESS_SCRIPT and MOZHARNESS_CONFIG in your task definition. You should be able to more or less copy some of the existing examples for this. Does that make sense? Let me know if you have other questions.
Flags: needinfo?(ahalberstadt)
Also, rather than add a new marionette-harness-test command, what would you think about hooking up pytest support to |mach python-test|? Or possibly creating a |mach pytest| instead? That way we'd run |mach python-test testing/marionette| instead of having N new commands per harness that most people don't really care about.
I will fix-up this commit to incorporate feedback and take advantage of the new "generic" TC tasks Review commit: https://reviewboard.mozilla.org/r/35587/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35587/
Depending on how MarionetteHarness is called, attributes like logger and pydebugger may not have been initialized. Review commit: https://reviewboard.mozilla.org/r/35589/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35589/
These tests aim to exercise the way MarionetteHarness is used in mach versus runtests.cli and downstream custom harnesses. Review commit: https://reviewboard.mozilla.org/r/35591/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35591/
If the appropriate test_handler isn't available to the runner, tests specified at the command-line silently omitted. It's possible for the runner to create an empty test-suite. This patch adds asserts to detect an empty test-suite early and provide a more specific error message. Review commit: https://reviewboard.mozilla.org/r/35593/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35593/
BaseMarionetteTestRunner sets up an empty list of test_handlers; however it is essential for any child class have at least one test_handler, otherwise the harness is busted. So, I added a test to check that MarionetteTestRunner sets up the expected test_handlers. In doing so, I refactor BaseMarionetteTestRunner.__init__ to split out file i/o into other methods so I can mock them out, which involves modifying testvars code. So, I also added a test to make sure the parsing of testvars still works. Review commit: https://reviewboard.mozilla.org/r/35597/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35597/
https://reviewboard.mozilla.org/r/35583/#review32277 ::: testing/marionette/client/marionette/tests/pytest-requirements.txt:1 (Diff revision 1) > +py==1.4.31 nit: white space
https://reviewboard.mozilla.org/r/35593/#review32443 ::: testing/marionette/client/marionette/tests/harness_unit/test_marionette_runner.py:166 (Diff revision 1) > + #test_file = "blah that uses MarionetteTestCase" this appears to be commnented out. just an FYI for because toy said you had something locally.
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/1-2/
Attachment #8721125 - Flags: feedback?(dburns)
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/1-2/
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/1-2/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/1-2/
Attachment #8721128 - Flags: feedback?(dburns)
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/1-2/
Attachment #8721129 - Flags: feedback?(dburns)
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/1-2/
Attachment #8721130 - Flags: feedback?(dburns)
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/1-2/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/1-2/
The "feedback" workflow isn't ideal for MozReview, so I'm pushing again with review flags set. Sorry for the noise.
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/2-3/
Attachment #8721125 - Attachment description: MozReview Request: Bug 1227367 - Add harness unit tests to marionette-client → MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r?automatedtester
Attachment #8721125 - Flags: review?(dburns)
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/2-3/
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/2-3/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/2-3/
Attachment #8721128 - Attachment description: MozReview Request: Bug 1227367 - Add checks to avoid AttributeErrors → MozReview Request: Bug 1227367 - Add checks to avoid AttributeErrors; r?automatedtester
Attachment #8721128 - Flags: review?(dburns)
Attachment #8721129 - Attachment description: MozReview Request: Bug 1227367 - Add tests for MarionetteHarness → MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r?automatedtester
Attachment #8721129 - Flags: review?(dburns)
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/2-3/
Attachment #8721130 - Attachment description: MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner → MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r?automatedtester
Attachment #8721130 - Flags: review?(dburns)
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/2-3/
Attachment #8721131 - Attachment description: MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture → MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r?automatedtester
Attachment #8721131 - Flags: review?(dburns)
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/2-3/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/2-3/
Attachment #8721132 - Attachment description: MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars → MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r?automatedtester
Attachment #8721132 - Flags: review?(dburns)
Attachment #8721125 - Flags: review?(dburns) → review+
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester https://reviewboard.mozilla.org/r/35583/#review32517
Attachment #8721128 - Flags: review?(dburns) → review+
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps https://reviewboard.mozilla.org/r/35589/#review32519
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester https://reviewboard.mozilla.org/r/35591/#review32521
Attachment #8721129 - Flags: review?(dburns) → review+
Attachment #8721130 - Flags: review?(dburns) → review+
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester https://reviewboard.mozilla.org/r/35593/#review32523
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester https://reviewboard.mozilla.org/r/35595/#review32525
Attachment #8721131 - Flags: review?(dburns) → review+
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester https://reviewboard.mozilla.org/r/35597/#review32527
Attachment #8721132 - Flags: review?(dburns) → review+
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/3-4/
Attachment #8721125 - Attachment description: MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r?automatedtester → MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/3-4/
Attachment #8721126 - Attachment description: MozReview Request: Bug 1227367 - Add tentative marionette-harness-test mach command → MozReview Request: Bug 1227367 - Add pytest command to mach; r?gps
Attachment #8721126 - Flags: review?(gps)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/3-4/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/3-4/
Attachment #8721128 - Attachment description: MozReview Request: Bug 1227367 - Add checks to avoid AttributeErrors; r?automatedtester → MozReview Request: Bug 1227367 - Add checks to avoid AttributeErrors; r=automatedtester
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/3-4/
Attachment #8721129 - Attachment description: MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r?automatedtester → MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/3-4/
Attachment #8721130 - Attachment description: MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r?automatedtester → MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/3-4/
Attachment #8721131 - Attachment description: MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r?automatedtester → MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/3-4/
Attachment #8721132 - Attachment description: MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r?automatedtester → MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps https://reviewboard.mozilla.org/r/35585/#review33573 We already have `mach python-test`. I'm not too keen on adding another mach command that is nearly the same. AFAIK this is also the first use of pytest in tree. Must we introduce pytest? What's wrong with unittest?
Attachment #8721126 - Flags: review?(gps)
Thanks for the feedback. We're keen to introduce pytest because it provides better separation between fixtures and test cases, which is especially nice for testing Marionette's unittest-based test harness. Would using a different name for the mach command ease your concerns? Say, mach python-pytest, to better distinguish from mach python-test?
Flags: needinfo?(gps)
(Please reply to MozReview comments on MozReview so it can capture threading. But since you replied here, I'll reply here. And yes, there is a bug on file to have Bugzilla enforce this.) If you say you really want to use pytest, I'm not going to say no. I would like to point out that we've made due with unittest-based tests up to this point. Multiple test harnesses can add cognitive overhead and make authoring and comprehending tests more difficult. See the train wreck that is the N forms of JavaScript tests that Firefox developers have to maintain. I have a bigger concern with the redundant mach commands. The way `mach python-test` works now is literally to invoke `python <file>` for every test file passed in. We rely on the file having a "if __name__ == '__main__'" that invokes an appropriate test runner. For things in mozilla-central, that's often mozunit, which is a shim around the unittest runner. I'd strongly prefer we do something similar to pytest based tests. Can we do that? At one point, I had a patch series that used nose to run various Python tests. I'd still like to use something like nose (even pytest!) because it is a much superior tool to the bare unittest runner (or even mozunit). But this work is scope bloat. Also, we should consider vendoring pytest in tree. It may not be strictly required. But we tend to vendor "important" Python dependencies to minimize variance over time and reliance on 3rd party servers. Lesser used things and processes not performed in automation typically don't get vendored. e.g. Sphinx and dependencies for `mach doc`.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #55) > (Please reply to MozReview comments on MozReview so it can capture > threading. But since you replied here, I'll reply here. And yes, there is a > bug on file to have Bugzilla enforce this.) Will do, next time. > I have a bigger concern with the redundant mach commands. The way `mach > python-test` works now is literally to invoke `python <file>` for every test > file passed in. We rely on the file having a "if __name__ == '__main__'" > that invokes an appropriate test runner. For things in mozilla-central, > that's often mozunit, which is a shim around the unittest runner. I'd > strongly prefer we do something similar to pytest based tests. Can we do > that? Yes! I will submit a patch shortly that uses a similar approach to invoke pytest via python-test. > Also, we should consider vendoring pytest in tree. It may not be strictly > required. But we tend to vendor "important" Python dependencies to minimize > variance over time and reliance on 3rd party servers. Lesser used things and > processes not performed in automation typically don't get vendored. e.g. > Sphinx and dependencies for `mach doc`. This is going to be used in automation, so I filed Bug 1253359.
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/4-5/
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/4-5/
Attachment #8721126 - Attachment description: MozReview Request: Bug 1227367 - Add pytest command to mach; r?gps → MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r?automatedtester;gps
Attachment #8721126 - Flags: review?(gps)
Attachment #8721126 - Flags: review?(dburns)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/4-5/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/4-5/
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/4-5/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/4-5/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/4-5/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/4-5/
Attachment #8721126 - Flags: review?(dburns) → review+
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps https://reviewboard.mozilla.org/r/35585/#review34793
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps https://reviewboard.mozilla.org/r/35585/#review35077 Per my review in bug 1253359, please change this back to installing pytest dynamically at run-time. Sorry for the confusion. Please badger me when the new patch is up for review: this series has lingered far too long waiting on my review and I don't want to delay you further.
Attachment #8721126 - Flags: review?(gps)
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/5-6/
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/5-6/
Attachment #8721126 - Attachment description: MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r?automatedtester;gps → MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps
Attachment #8721126 - Flags: review?(gps)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/5-6/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/5-6/
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/5-6/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/5-6/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/5-6/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/5-6/
Attachment #8721126 - Flags: review?(gps) → review+
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps https://reviewboard.mozilla.org/r/35585/#review35149 LGTM!
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/43403/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43403/
Attachment #8721128 - Attachment description: MozReview Request: Bug 1227367 - Add checks to avoid AttributeErrors; r=automatedtester → MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps
Attachment #8721127 - Attachment description: MozReview Request: Bug 1227367 - Add TaskCluster harness unit test task → MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin
Attachment #8736575 - Flags: review?(gps)
Attachment #8721128 - Flags: review?(gps)
Attachment #8721127 - Flags: review?(dustin)
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/6-7/
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35585/diff/6-7/
Comment on attachment 8721128 [details] MozReview Request: Bug 1227367 - Report when mach python-tests collects no tests; r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35589/diff/6-7/
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/6-7/
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/6-7/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/6-7/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/6-7/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/6-7/
Comment on attachment 8721126 [details] MozReview Request: Bug 1227367 - Make harness tests compatible with mach python-test; r=automatedtester; r?gps Manually setting review flag because Mozreview thinks this mach python-test patch has already been approved, but un-bitrotting requires significant changes.
Attachment #8721126 - Flags: review+ → review?(gps)
Attachment #8721127 - Flags: review?(dustin)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin https://reviewboard.mozilla.org/r/35587/#review40043 A bunch of comments, but they're all minor -- this is definitely the right approach. Thanks! ::: testing/docker/desktop-build/bin/build.sh:6 (Diff revision 7) > #! /bin/bash -vex > > set -x -e -v > > +# Relative path to in-tree script to execute instead of build-linux.sh > +: JOB_SCRIPT ${JOB_SCRIPT} Rather than including a conditional at the bottom, I'd prefer to just make this variable default to `testing/taskcluster/scripts/builder/build-linux.sh` here. ::: testing/taskcluster/tasks/branches/base_jobs.yml:321 (Diff revision 7) > when: > file_patterns: > - 'testing/mozharness/**' > + marionette-harness: > + task: tasks/tests/harness_marionette.yml > + # "Root tasks are scheduled immediately, if scheduled to run." I think this is clear from context (at least, it's used several places in this file without explanation) and should be omitted. ::: testing/taskcluster/tasks/harness_test.yml:16 (Diff revision 7) > + metadata: > + source: '{{source}}' > + owner: mozilla-taskcluster-maintenance@mozilla.com > + tags: > + createdForUser: {{owner}} > + workerType: b2gtest Please use `desktop-test-xlarge` ::: testing/taskcluster/tasks/tests/harness_marionette.yml:23 (Diff revision 7) > + - --work-dir=mozharness_workspace > + env: > + JOB_SCRIPT: 'testing/taskcluster/scripts/tester/harness-test-linux.sh' > + MOZHARNESS_SCRIPT: > > + testing/mozharness/scripts/marionette_harness_tests.py > + TOOLS_DISABLE: true Why is `TOOLS_DISABLE` defined here and not in `harness_test.yml`?
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/7-8/
Attachment #8721127 - Flags: review?(dustin)
Comment on attachment 8721129 [details] MozReview Request: Bug 1227367 - Add tests for MarionetteHarness; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35591/diff/7-8/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/7-8/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/7-8/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/7-8/
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin https://reviewboard.mozilla.org/r/35587/#review40147
Attachment #8721127 - Flags: review?(dustin)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin https://reviewboard.mozilla.org/r/35587/#review40153
Attachment #8721127 - Flags: review+
https://reviewboard.mozilla.org/r/35585/#review35149 While I've sort-of worked around Ted's changes to ./mach python-test (Bug 1255479), I really think it makes more sense to just go back to the ./mach pytest solution instead, given how entangled PYTHON_UNIT_TESTS and ./mach python-test are with build config. Are you open to that option?
Comment on attachment 8721125 [details] MozReview Request: Bug 1227367 - Test exit codes in marionette-client runtests.cli; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35583/diff/7-8/
Comment on attachment 8736575 [details] MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43403/diff/1-2/
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/8-9/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/8-9/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/8-9/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/8-9/
Attachment #8721126 - Attachment is obsolete: true
Attachment #8721126 - Flags: review?(gps)
Attachment #8721128 - Attachment is obsolete: true
Attachment #8721128 - Flags: review?(gps)
Attachment #8721129 - Attachment is obsolete: true
Comment on attachment 8736575 [details] MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43403/diff/2-3/
Attachment #8736575 - Attachment description: MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?gps → MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund
Attachment #8736575 - Flags: review?(gps) → review?(jlund)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/9-10/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/9-10/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/9-10/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/9-10/
Comment on attachment 8736575 [details] MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund https://reviewboard.mozilla.org/r/43403/#review40829 looks great. some questions below before stamping. ::: testing/mozharness/scripts/marionette_harness_tests.py:82 (Diff revision 3) > + ) > + > + def clobber(self): > + """Delete the working directory""" > + super(MarionetteHarnessTests, self).clobber() > + print "bllllla" debug line? actually, I don't think you need to overwrite clobber() at all. just let the method resolution order find the parent clobber() ::: testing/mozharness/scripts/marionette_harness_tests.py:137 (Diff revision 3) > + import pytest > + log_path = os.path.join(dirs['abs_log_dir'], 'pytest.log') > + command = ['--resultlog', log_path, '--verbose', test_path] > + self.info('Calling pytest.main with the following arguments: %s' % command) > + status = self._get_pytest_status(pytest.main(command)) > + self.run_command(['cat', log_path]) maybe use read_from_file() here instead of run_command and subprocess call so we control file handling: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#773 ::: testing/mozharness/scripts/marionette_harness_tests.py:138 (Diff revision 3) > + log_path = os.path.join(dirs['abs_log_dir'], 'pytest.log') > + command = ['--resultlog', log_path, '--verbose', test_path] > + self.info('Calling pytest.main with the following arguments: %s' % command) > + status = self._get_pytest_status(pytest.main(command)) > + self.run_command(['cat', log_path]) > + self.buildbot_status(status) do you need this?
Attachment #8736575 - Flags: review?(jlund)
https://reviewboard.mozilla.org/r/43403/#review40829 > debug line? > > actually, I don't think you need to overwrite clobber() at all. just let the method resolution order find the parent clobber() whoops! I forgot to delete that whole method. > do you need this? Yes, but I'm really only using it to set `self.return_code` appropriately.
Comment on attachment 8736575 [details] MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43403/diff/3-4/
Attachment #8736575 - Flags: review?(jlund)
Comment on attachment 8721127 [details] MozReview Request: Bug 1227367 - Add marionette-harness TaskCluster task; r?dustin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35587/diff/10-11/
Comment on attachment 8721130 [details] MozReview Request: Bug 1227367 - Detect empty test suite in BaseMarionetteTestRunner; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35593/diff/10-11/
Comment on attachment 8721131 [details] MozReview Request: Bug 1227367 - Set logger fixture as a dependency for the kwargs fixture; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35595/diff/10-11/
Comment on attachment 8721132 [details] MozReview Request: Bug 1227367 - Test MarionetteTestRunner's test_handlers and testvars; r=automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35597/diff/10-11/
Comment on attachment 8736575 [details] MozReview Request: Bug 1227367 - Add mozharness script for Marionette harness tests; r?jlund https://reviewboard.mozilla.org/r/43403/#review40867 awesome. looks good!
Attachment #8736575 - Flags: review?(jlund) → review+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: