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)

enhancement
Not set
normal

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
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.
Product: Core → Firefox Build System
Attached patch squash pytest warnings about Test* classes (obsolete) (deleted) — Splinter Review
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)
Assignee: nobody → nfroyd
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
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+
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
[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)
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 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?
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?
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.
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.
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)
(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|.
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 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+
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
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5a40f58fc3c9 [python-test] Use a global pytest.ini configuration file, r=davehunt
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(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)
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: