1253359 - Vendor in pytest 2.9.2 and py 1.4.31

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=1227367#c55 for context.

Comment 1

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

Attachment: MozReview Request: Bug 1253359 - Vendor in pytest 2.9.0 and py 1.4.31; r?gps

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8726453 [details]
MozReview Request: Bug 1253359 - Vendor in pytest 2.9.0 and py 1.4.31; r?gps

https://reviewboard.mozilla.org/r/38009/#review35073

We talked about pytest in the build system meeting last week. While many of us like pytest and think there may be a future where it is more tightly integrated with the build system (we like the idea of making pytest the harness through which we run python tests in the tree), given its limited use at this time, we would like to hold off vendoring it until there are more consumers.

For the time being, please install pytest and friends at run-time when mach commands are invoked.

Sorry for changing my stance on this.

Comment 3

[James Graham [:jgraham]]

FWIW we are going to want something for running webdriver-spec tests in automation; the current code uses nose, but it might be possible to port to py.test. Porting to unittest is quite undesirable, having seen what a disaster that is in other harnesses. So that's one more consumer already.

Comment 4

[James Graham [:jgraham]]

And I really really don't want to rely on the local pypi mirror in automation. That has previously been a terrible experience.

Comment 5

[Andreas Tolfsen ❲:ato❳]

The forthcoming other potential consumer that :jgraham speaks of are the Web Platform Tests for the WebDriver specification.  The harness needs a Python test runner and there are a few options:

  * Use pytest to drive the tests
  * Wrap unittest directly
  * Write a home-brewed harness that mimics pytest test style

What Mozilla and the other browser vendors decide restricts the choices available for WPT.  If Mozilla decides it absolutely does not want to vendor pytest, we need to consider the two latter options, which are both worse but for different reasons:

Integrating unittest is a lot of work because of how its APIs have been designed.  The testing style also does not lend itself particularly well to what the specification tests are trying to do.  Writing a custom runner might or might not be more work, but isn’t helpful for consolidation.

The harness is being implemented as we speak in https://github.com/w3c/wptrunner/pull/171.

Comment 6

[Gregory Szorc [:gps]]

We're not strongly opposed to pytest: we just didn't see a good reason to vendor it at this time. If we need to vendor pytest, we will.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Small update on this: WPT dependencies are vendored in tools/, and https://github.com/w3c/wpt-tools/commit/5498c7440517a3399dfaf7c6c1cb2a5792b71b4c adds pytest to this directory through a submodule.

Comment 8

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

Another update: we're considering adding a pytest plugin, (https://github.com/davehunt/pytest-mozlog), to m-c, somewhere under testing/mozbase. So far, the discussion favours including the plugin in mozlog, but then mozlog would depend on pytest. 

We considered removing the pytest dependency from the plugin to avoid the issue, but that's kind of a hack that interferes with usual pytest-plugin expectations. 

The plugin would also be used by out-of-tree projects by WebQA. In tree, it is needed for the Marionette Harness Tests (See Bug 1285299).

So, what do you think of vendoring pytest?

Comment 9

[Gregory Szorc [:gps]]

Sold. Let's vendor pytest.

Comment 10

[Anjana Vakil (:vakila)]

Attachment: Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

Vendor in Pytest (2.9.2) and its requirement Py (1.4.31),
so that it can be used for e.g. the Marionette
harness unit tests and a pytest plugin for mozlog.

Copy pytest and py package directories (extracted from
tars on Pip) into `mozilla-central/python/`.
Add both `.pth` entries to `virtualenv_packages.txt`.
Add both paths to `SEARCH_PATHS` in `mach_bootstrap.py`.

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

Comment 11

[Anjana Vakil (:vakila)]

Attachment: Bug 1253359 - Use vendored Pytest in `python-test` and Mn harness tests

In the `python-test` mach command and the mozharness script for
the Marionette harness tests, use the vendored-in Pytest
instead of installing from pip.

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

Comment 12

[Anjana Vakil (:vakila)]

Comment on attachment 8726453 [details]
MozReview Request: Bug 1253359 - Vendor in pytest 2.9.0 and py 1.4.31; r?gps

Marking Maja's previous patch with older Pytest version as obsolete, the first of the two patches I just submitted reproduces this one but with Pytest 2.9.2

Comment 13

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

https://reviewboard.mozilla.org/r/64578/#review61634

::: python/mach_commands.py:93
(Diff revision 1)
>          # clashing namespaces when importing multiple test modules. What follows
>          # is a simple way to keep environments separate, at the price of
>          # launching Python multiple times. Most tests are run via mozunit,
>          # which produces output in the format Mozilla infrastructure expects.
>          # Some tests are run via pytest, and these should be equipped with a
>          # local mozunit_report plugin to meet output expectations.

Remove 'and these should...'.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

https://reviewboard.mozilla.org/r/64576/#review61682

There's a *lot* of support files in here, especially for pytest. The bloat is too much for my liking.

Can we narrow things down to just the source files?

If you download the .whl from https://pypi.python.org/pypi/pytest, you can `unzip` it to python/pytest and it should "just work." Ditto for py.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8771354 [details]
Bug 1253359 - Use vendored Pytest in `python-test` and Mn harness tests

https://reviewboard.mozilla.org/r/64578/#review61690

Comment 16

[Anjana Vakil (:vakila)]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

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

Comment 17

[Anjana Vakil (:vakila)]

Comment on attachment 8771354 [details]
Bug 1253359 - Use vendored Pytest in `python-test` and Mn harness tests

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

Comment 18

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/64576/#review61682

Sorry for the slow reply - I was away the last 2 weeks - but I've redone the patch to use the unzipped wheels instead. Hope it looks better, but let me know if there's any more bloat in there that I should get rid of.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

https://reviewboard.mozilla.org/r/64576/#review65392

Comment 20

[Anjana Vakil (:vakila)]

Attachment: Bug 1253359 - Add requirements file to Mn-h task watch list

Changes to the Marionette harness test requirements should trigger
the marionette-harness (Mn-h) job to run. Add the requirements
file to the file_patterns in the definition of the
marionette-harness job.

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

Comment 21

[Anjana Vakil (:vakila)]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

https://reviewboard.mozilla.org/r/64576/#review61682

So, it appears that the unzipped wheels don't work when the Marionette harness job runs on TaskCluster, because pip is expecting a `setup.py` file in the directory it's trying to install from:

https://public-artifacts.taskcluster.net/R5WTsiA9RM2wdU6wNhpWkw/0/public/logs/live_backing.log

Should we go back to the original directory, but manually removing the support files and just leaving the source? 

Or am I missing some obvious/simple solution?

Comment 22

[Anjana Vakil (:vakila)]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

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

Comment 23

[Anjana Vakil (:vakila)]

Comment on attachment 8771354 [details]
Bug 1253359 - Use vendored Pytest in `python-test` and Mn harness tests

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

Comment 24

[Andrew Halberstadt [:ahal]]

Leaving the .whl unextracted should also work. But then we can't search the source files in m-c, which personally I don't think is important, but some might disagree.

I don't think you're missing anything, pip doesn't seem to be able to install extracted wheels.

Comment 25

[Anjana Vakil (:vakila)]

(In reply to Andrew Halberstadt [:ahal] from comment #24)
> Leaving the .whl unextracted should also work. But then we can't search the
> source files in m-c, which personally I don't think is important, but some
> might disagree.

Thanks for the suggestion - I had thought about that (using unextracted wheels), but as you said I thought it might be good to have access to the source, especially if we're going to be messing about with plugins (e.g. pytest-mozlog) etc. 

I'd also thought about copying the .whl archive inside the extracted archive, so that we have both, but decided that would be a bit confusing. 

So I decided to just manually go through and remove certain support files (docs, changelog, tests...) from the original (untarred) directory. Works locally, and just ran it on try and Taskcluster seems to be OK with it too. Also, that seems to be how it's been done for most of the other packages in `python/`, perhaps it's best to be consistent.

Comment 26

[Anjana Vakil (:vakila)]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

Since the extracted wheel wasn't working for Taskcluster, I went back and just removed as much as seemed reasonable from the original (untarred) directories. Seems to work locally and on Taskcluster now.  

Resetting the r? flag because of the change. Hopefully now the dirs are small/bloat-free enough? 

If not, let me know and I'll try another approach (e.g. unextracted wheels as discussed in the comment above).

Comment 27

[Anjana Vakil (:vakila)]

Comment on attachment 8771354 [details]
Bug 1253359 - Use vendored Pytest in `python-test` and Mn harness tests

Decided to include the Marionette harness test requirements file in the list of watched files for the Mn-h job, a) to get it to run the job to test these patches and b) because the job should probably run whenever the requirements change. Seem reasonable?

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

https://reviewboard.mozilla.org/r/64576/#review68186

The dist-info directories can be removed if you want.

Follow the discussion on the bug, we shouldn't check .whl files into the tree because they are glorified zip files.

Comment 29

[Anjana Vakil (:vakila)]

Comment on attachment 8771353 [details]
Bug 1253359 - Vendor in Pytest 2.9.2 and Py 1.4.31

https://reviewboard.mozilla.org/r/64576/#review68186

Hm, I had already removed the dist-info directories in the latest patch... not sure why they're showing up as "new file"s in MozReview when viewing the diff between revisions 2 and 3, but viewing the diff between original and revision 3 (latest), they're not there (as expected). The only remaining dist-info is inside `_pytest/vendored-packages/`, which I'm OK with leaving in.

Thanks for the reviews/feedback - since everyone seems OK with this now I'll try to find someone with access to land it.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62f855e87250
Vendor in Pytest 2.9.2 and Py 1.4.31 r=gps
https://hg.mozilla.org/integration/autoland/rev/47d2e5c4d8fb
Use vendored Pytest in `python-test` and Mn harness tests r=gps

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62f855e87250
https://hg.mozilla.org/mozilla-central/rev/47d2e5c4d8fb