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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P5)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[David Burns :automatedtester]

[mass update] Setting priority

Comment 2

[Benjamin De Brasi]

Hi, can I work on this bug?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Benjamin De Brasi]

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

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 6

[Andrew Halberstadt [: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')

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, ok. Andreas, which specific tests did you have in mind here?

Comment 8

[Andreas Tolfsen ❲: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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sure, so you want to see it for all of them?

Comment 10

[Benjamin De Brasi]

Is it for all the tests?

Comment 11

[Andreas Tolfsen ❲: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.

Comment 12

[Andrew Halberstadt [:ahal]]

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 ...

Comment 13

[Benjamin De Brasi]

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

Comment 14

[Andreas Tolfsen ❲: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".

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Benjamin were you able to successfully test your changes?

Comment 16

[Petru Gurita]

Attachment: Bug 1319793 - [marionette harness tests] Default to not swallow stdout from pytest.

Added the flag 'capture --no' to the harness_unit tests. Stdout is now displayed by default.

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

Comment 17

[Petru Gurita]

Hello, I think I got a working solution. I've tested with print statements and everything works fine :).

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 19

[Petru Gurita]

I know, I know... It's just that I did not find something else suitable for me... Sorry.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 22

[Andreas Tolfsen ❲: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.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8994747 [details]
Bug 1319793 - [marionette harness tests] Default to not swallow stdout from pytest.

https://reviewboard.mozilla.org/r/259278/#review267692

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb25e87905b1
[marionette harness tests] Default to not swallow stdout from pytest. r=whimboo

Comment 26

[Andreas Tolfsen ❲:ato❳]

Thanks for fixing this minor annoyance!

Comment 27

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/cb25e87905b1