Closed
Bug 1417920
Opened 7 years ago
Closed 6 years ago
Silence pytest warnings about build system classes whose names start with `Test`
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: ted, Assigned: ahal)
References
Details
Attachments
(1 file, 1 obsolete file)
Running Python tests for various build system tests winds up spitting out warnings like:
0:02.68 =============================== warnings summary ===============================
0:02.68 mozbuild/test/backend/test_recursivemake.py::TestManifestBackend
0:02.68 cannot collect test class 'TestManifestBackend' because it has a __init__ constructor
0:02.68
0:02.68 -- Docs: http://doc.pytest.org/en/latest/warnings.html
I guess this is because pytest expects every class in the file whose name starts with `Test` to be tests, but we have various classes in the build system that represent test-related things that are just named that way. We should silence these warnings.
I think davehunt noted that we could add some magic class property to silence these, but I lost it in IRC scrollback and didn't have the patience to go look for it.
In addition to `TestManifestBackend` we would need to apply the fix to at least `TestManifest` and `TestHarnessFiles`, which show up as warnings when running test_emitter:
0:02.51 mozbuild/test/frontend/test_emitter.py::TestManifest
0:02.51 cannot collect test class 'TestManifest' because it has a __init__ constructor
0:02.51
0:02.51 mozbuild/test/frontend/test_emitter.py::TestHarnessFiles
0:02.51 cannot collect test class 'TestHarnessFiles' because it has a __init__ constructor
Comment 1•7 years ago
|
||
From https://github.com/pytest-dev/pytest/blob/master/CHANGELOG.rst#310-2017-05-22:
> It is now possible to skip test classes from being collected by setting a __test__ attribute to False in the class body.
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 3•6 years ago
|
||
Having an imported class that begins with `Test` causes pytest to warn:
=================================== warnings summary ====================================
mozbuild/test/backend/test_recursivemake.py::TestMetadataBackend
cannot collect test class 'TestMetadataBackend' because it has a __init__ constructor
-- Docs: http://doc.pytest.org/en/latest/warnings.html
After seeing this several times and each time wondering what the yellow
output in your terminal might mean, the warning starts to get tiresome.
A class-scope `__test__` attribute set to `False` makes this warning go
away; let's do that and eliminate this papercut.
Attachment #8979686 -
Flags: review?(ted)
Updated•6 years ago
|
Assignee: nobody → nfroyd
Assignee | ||
Comment 4•6 years ago
|
||
Another option would be to disable class based discovery altogether by using:
[pytest]
python_classes =
Afaik, all our pytests are either unittest.TestCase based (which are unaffected by the python_classes config), or are module level methods. So I don't think disabling this discovery feature would have any impact.
If we still wanted to support native pytest class-based tests, we could just change the search prefix:
[pytest]
python_classes = PyTest
Reporter | ||
Comment 5•6 years ago
|
||
Comment on attachment 8979686 [details] [diff] [review]
squash pytest warnings about Test* classes
Review of attachment 8979686 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, this is annoying! ahal: if you want to fix things that way that's fine, but I don't understand pytest well enough to comment usefully on your suggestion.
Attachment #8979686 -
Flags: review?(ted) → review+
Assignee | ||
Comment 6•6 years ago
|
||
My comment mid-aired with the patch, this fix is likely good enough and I'm not motivated enough to do it the other way.
But for posterity, if we added "python_classes = PyTest" to the pytest config, then only classes that were prefixed with "PyTest" would be considered to contain tests. Similarly, leaving that blank would disable class based discovery altogether.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2491b63fe79a
squash pytest warnings about Test* classes; r=ted.mielczarek
Comment 8•6 years ago
|
||
Backed out changeset 2491b63fe79a (bug 1417920) for build bustages
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b4f255a1160eb87c44bb04ff1b22240165d7e2
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5fd3144d5d009fdac3fae2ee33143901164e0d12&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&selectedJob=182107660
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=182107660&repo=mozilla-inbound&lineNumber=37269
Flags: needinfo?(nfroyd)
Comment 9•6 years ago
|
||
[task 2018-06-06T15:21:19.466Z] 15:21:19 INFO - Return code from mach python-test: 5
[task 2018-06-06T15:21:19.679Z] 15:21:19 INFO - /builds/worker/workspace/build/src/testing/testsuite-targets.mk:259: recipe for target 'check' failed
[task 2018-06-06T15:21:19.679Z] 15:21:19 INFO - make: *** [check] Error 5
I have no idea what this means. Maybe that means we should go with comment 6?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 10•6 years ago
|
||
Sure, hope you don't mind if I steal this. Been meaning to get a pytest.ini added anyway.
Assignee: nfroyd → ahal
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8983909 [details]
Bug 1417920 - [python-test] Use a global pytest.ini configuration file,
https://reviewboard.mozilla.org/r/249758/#review256106
::: config/mozunit/mozunit/mozunit.py:237
(Diff revision 1)
> if os.environ.get('MACH_STDOUT_ISATTY') and not any(a.startswith('--color') for a in args):
> args.append('--color=yes')
>
> module = __import__('__main__')
> args.extend([
> + '-c', os.path.join(here, 'pytest.ini'),
pytest looks for configuration files, and we have a few already in the repository (for example https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/harness_unit/pytest.ini). does setting this in mozunit cause pytest to not discover the ones we have?
Assignee | ||
Comment 13•6 years ago
|
||
Good catch. Looks like pytest doesn't support configuration inheritance, it'll only load a single config file. So yes, doing this will turn off that config file.
Tbh I question why we should allow tests to define their own configs, I'd prefer all tests run consistently. But maybe there's a good reason the marionette harness tests need to disable the terminal reporter?
Anyway, I think the marionette tests could also accomplish this by passing in -p no:terminalreporter on the command line. Also that is the only pytest.ini we have running with |mach python-test| (the others are all in third party code).
So I could delete that pytest.ini and pass in no:terminalreporter via command line instead. Do you have any other ideas?
Comment 14•6 years ago
|
||
We also have: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/pytest.ini
I agree that we should have consistency, and a single pytest.ini for all of the tests run under |mach python-test|. Note that I believe the pytest root directory is set from the location of the configuration file, which I think affects the test paths in the output (so a pytest.ini in the topsrcdir would list tests with their path relative to root). I also think this is a good thing, but worth noting in case it affects any intermittent bug associations or similar.
I think marionette harness tests are attempting to match tbpl output by using the mozlog tbpl reporter (via a command line option in the call to mozunit) and disabling the terminal reporter plugin. I think doing this would mean less need for some of logging in `run_python_tests`, but I'd still vote for consistency.
Assignee | ||
Comment 15•6 years ago
|
||
Hrm, passing -p no:terminalreporter on the command line doesn't seem to work :/. I think the marionette tests need this option because they're using pytest-mozlog instead. Maybe we can just get them to use the regular pytest format like everything else.
Assignee | ||
Comment 16•6 years ago
|
||
Hey Andreas,
To summarize, I'm trying to add a global pytest.ini file that will apply to all python tests. But doing so will disable this file here:
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/harness_unit/pytest.ini
For some reason, I'm having trouble moving the "-p no:terminalreporter" option to the command line. So before I spend a lot of time looking into this, I wanted to ask you if you cared whether the marionette harness tests use the tbpl format over the default pytest format. Can we stop using --log-tbpl=- for these tests?
Flags: needinfo?(ato)
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #14)
> We also have:
> https://searchfox.org/mozilla-central/source/testing/web-platform/tests/
> tools/pytest.ini
I think that is also third party. At the very least, those tests aren't hooked up to |mach python-test|.
Assignee | ||
Comment 18•6 years ago
|
||
Oh nvm. Looks like I just need to pass in ['-p', 'no:terminalreporter'] as opposed to ['-p=no:terminalreporter'].
Andreas, I'll leave the needinfo anyway, but there is now a path forward here regardless.
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8983909 [details]
Bug 1417920 - [python-test] Use a global pytest.ini configuration file,
https://reviewboard.mozilla.org/r/249758/#review256308
Attachment #8983909 -
Flags: review?(dave.hunt) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Comment on attachment 8979686 [details] [diff] [review]
squash pytest warnings about Test* classes
Just waiting on one last try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61cedca4792719dc970fc7756fb960cabc0abdb6
Attachment #8979686 -
Attachment is obsolete: true
Comment 22•6 years ago
|
||
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a40f58fc3c9
[python-test] Use a global pytest.ini configuration file, r=davehunt
Comment 23•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 24•6 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #16)
> To summarize, I'm trying to add a global
> pytest.ini file that will apply to all python
> tests. But doing so will disable this file here:
> https://searchfox.org/mozilla-central/source/testing/marionette/ha
> rness/ marionette_harness/tests/harness_unit/pytest.ini
>
> For some reason, I'm having trouble moving the "-p
> no:terminalreporter" option to the command line. So before I spend
> a lot of time looking into this, I wanted to ask you if you cared
> whether the marionette harness tests use the tbpl format over the
> default pytest format. Can we stop using --log-tbpl=- for these
> tests?
This is more a question for maja_zf (cc’ed) than me, but I don’t
see a problem using the default pytest format. I don’t expect this
will interfere with Treeherder’s ability to parse the test results?
Flags: needinfo?(ato)
Assignee | ||
Comment 25•6 years ago
|
||
Thanks, good to know! Correct, treeherder would still show failures in the errorsummary.
Now that this bug is fixed, I'm not very motivated to file a new bug and change the format just for the sake of it. But maybe this is something we could do eventually to get all the tests running consistently.
Yeah, I think the tbpl format was applied to Marionette Harness Tests initially to fit with Treeherder's log parsing at the time. Without pytest-mozlog, the Treeherder error summary didn't show anything useful. If that's not the case anymore and log parsing for the default pytest format works, then turning off the tbpl-style logging is fine.
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•