Closed Bug 1319793 Opened 3 years ago Closed 2 years ago

[marionette harness tests] Default to not swallow stdout from pytest

Categories

(Testing :: Marionette, defect, P5)

Version 3
defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: petru.gurita1, Mentored)

Details

(Keywords: pi-marionette-harness-tests, Whiteboard: [lang=py])

Attachments

(1 file)

It is sometimes convenient to include stdout from tests in the shell.  We could pass the `--capture no` flag (equivalent to -s) to pytest in order to not swallow this output.

As a data point, we already do this for the WPT wdspec tests (in https://github.com/w3c/wptrunner/blob/master/wptrunner/executors/pytestrunner/runner.py#L54) and it has not been an annoyance.
[mass update] Setting priority
Priority: -- → P5
Hi, can I work on this bug?
Flags: needinfo?(ato)
Sure. As usual let me know if you have questions. Otherwise I will mark the bug assigned to you once the first path has been uploaded.
Mentor: hskupin
Flags: needinfo?(ato)
Whiteboard: [lang=py]
Hi, sorry for taking so long to start on this bug.

It seems the github repo linked to above has been relocated. I think I found what I'm supposed to be looking at, but I'd like to check to make sure:

https://github.com/w3c/web-platform-tests/blob/master/tools/wptrunner/wptrunner/executors/pytestrunner/runner.py
Flags: needinfo?(ato)
The canonical repository has not been relocated but we make use of a copy of it in our tree. But you have the correct file to look at in how we can fix it for the marionette harness tests.

Sadly I forgot in which file the python-tests invoke pytest, but Andrew will be able to tell it to us.
Flags: needinfo?(ato) → needinfo?(ahal)
Generally it would be set here:
https://searchfox.org/mozilla-central/source/config/mozunit/mozunit/mozunit.py

But I'd r- this change if it set the config globally for all python-tests. Instead you can pass extra arguments into pytest via the mozunit.main() call at the bottom of each test. E.g, mozunit.main('--capture=no')
Flags: needinfo?(ahal)
Hm, ok. Andreas, which specific tests did you have in mind here?
Flags: needinfo?(ato)
This is about the Marionette harness unit tests located under
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/harness_unit.
Flags: needinfo?(ato)
Sure, so you want to see it for all of them?
Is it for all the tests?
Flags: needinfo?(ato)
As I said in comment #0, I think it makes sense to surface all
stdout/stderr from all MnH (Marionette harness unit tests).

At the moment I don’t expect any of them have any print() statements
in them, but print() is by far the most powerful debugging technique
and it is detrimental not to have access to it.
Flags: needinfo?(ato)
I think it's totally fine to do this (for the MnH subset of tests), but just wanted to point out that:

1. Pytest does dump stdout of the test functions if it fails (I normally add assert False to the end when writing new tests)
2. You can pass in -s via the PYTEST_ADDOPTS environment variable, e.g:
PYTEST_ADDOPTS="-s" ./mach python-test ...
I believe I've fixed this bug. How can I test if the code I wrote works?
Flags: needinfo?(ato)
(In reply to Benjamin De Brasi from comment #13)

> I believe I've fixed this bug. How can I test if the code I wrote
> works?

Add some print() statements to a Python test file, for example
testing/marionette/harness/marionette_harness/tests/harness_unit/tes
t_httpd.py, run it in a shell and verify that its stdout contain the
lines you asked to print.  The file should be runnable with "./mach
python-test".
Flags: needinfo?(ato)
Benjamin were you able to successfully test your changes?
Flags: needinfo?(csdebugging)
Hello, I think I got a working solution. I've tested with print statements and everything works fine :).
Petru, thank you for the patch, but generally someone else should only work on a bug if the former one doesn't respond anymore. Given that I asked Benjamin only 2 days ago I would like to wait at least until Monday. If we don't hear from him by then, or he agrees that you can fix it, I will happily accept your patch and review it. I hope that this is ok for you.
I know, I know... It's just that I did not find something else suitable for me... Sorry.
(In reply to Petru Gurita from comment #19)
> I know, I know... It's just that I did not find something else suitable for me... Sorry.

In the future as best ask first before starting on a bug. This prevents clashes when several people are working on the same patch, and makes others work obsolete. 

But given that we didn't get any reply from Benjamin yet, I will assign the bug to you now. I will do the review later today. Thanks.
Assignee: nobody → petru.gurita1
Status: NEW → ASSIGNED
Flags: needinfo?(csdebugging)
So I had a look at the patch and also tried it locally. By doing so I have seen that those print lines always appear in my console whether if the patch is applied or not. Then I remembered that I set the `PYTEST_ADDOPTS=-s` environment variable a while ago, which is exactly the feature we need here, right?

Andreas, so if this request was only for local development I would propose that you set this environment variable. Otherwise please tell me for which specific situations you also want to have it.

Sorry that I forgot about the environment variable.
Flags: needinfo?(ato)
I think remembering to set that variable is not a great option.
The best option here is to have sensible defaults, and it makes
sense to include print() statements in the regular stdout.
Flags: needinfo?(ato)
If that is a sensible default we will see by just enabling it for the marionette harness unit tests. Thing is if that would be a general good default, why isn't it enabled by default for pytest and our build system. Anyway, lets see how this works for us.
Comment on attachment 8994747 [details]
Bug 1319793 - [marionette harness tests] Default to not swallow stdout from pytest.

https://reviewboard.mozilla.org/r/259278/#review267692
Attachment #8994747 - Flags: review?(hskupin) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb25e87905b1
[marionette harness tests] Default to not swallow stdout from pytest. r=whimboo
Thanks for fixing this minor annoyance!
https://hg.mozilla.org/mozilla-central/rev/cb25e87905b1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.