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)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ted, Assigned: ahal)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1417920 - [python-test] Use a global pytest.ini configuration file,
59 bytes, text/x-review-board-request
davehunt
: review+
Details 
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
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
https://hg.mozilla.org/mozilla-central/rev/5a40f58fc3c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
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.
See Also: → 1618981
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: