[mozlog] Move the stdio capture from web-platform-tests to mozlog.structured

RESOLVED FIXED in Firefox 67

Status

defect
RESOLVED FIXED
5 years ago
5 days ago

People

(Reporter: jgraham, Assigned: nikkis)

Tracking

unspecified
mozilla67
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

The ability to capture the parent process' stdio seems like a useful feature to move out into mozlog.strucured.
Summary: Move the stdio capture from web-platform-tests to mozlog.structured → [mozlog] Move the stdio capture from web-platform-tests to mozlog.structured
Assignee

Comment 1

4 months ago

Hey James, thanks for the reply on Bug 1066323! I'd like to work on the above to begin with, since there's an abundance of people working on the others. Thank you!
Cheers.

Assignee

Comment 2

4 months ago

Hi James, just wanted to clarify and make sure I am on the right track.

You would like capture_stdio in
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py
to be added, similarly, to
https://searchfox.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/structuredlog.py
?

The stdio capture used in TestRunner could be added in a similar way to the LoggerState, is that correct?
Please let me know if I am way off and need a little nudge in a different direction!

Thank you in advance!

Hi, sorry for not replying to this earlier, I was afk all weekend.

Yes, the idea is to take the CaptureIO class as used at [1] from [2] and put it into mozlog somewhere along with all the dependecies required to run it (probably in a new file somewhere). You probably also want to try writing some tests to ensure that it is working correctly after the move.

Does that make sense?

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py#141
[2] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/wptlogging.py#103-134

Assignee

Comment 4

4 months ago

Hi James. No worries! It was the weekend. :)

Thank you for getting back to me. This makes sense. Although, I may have some more specific questions to follow!

Looking at the CaptureIO class, it relies on the LogThread and LoggingWrapper classes that are in the same file. I am wondering if it would make sense to move those along with it?

Also, as testrunner.py[1] already imports part of mozlog anyway, it would be easy to just add the new capture.py file I'm creating in mozlog as another dependency. That would mean, once moved, the CaptureIO class currently in wptlogging.py[2] could be deleted safely to avoid duplication. Does that work?

Thanks again for your help!

yes, I imagine moving all the required code out of the wptlogging file and into mozlog. Then the wptlogging file should just import from mozlog instead of duplicating the code, just as you say.

Assignee

Comment 6

4 months ago

The ability to capture the parent process' stdio is suggested to be a useful feature
to move from web-platform/tests into mozlog. To do so, I have created a new capture.py
file within mozlog/mozlog. This includes the CaptureIO class and its dependencies,
including the LoggingWrapper and LogThread classes. These have been removed from their
original location, to avoid duplication, and the files depending on them updated
accordingly.

It would be useful to add unittests testing the CaptureIO enter and exit methods, and
the original_stdio, logging_queue and logging_thread properties. I have begun such a
file with test_capture.py in mozlog/tests. This is a work in progress, however I may
need some guidance, please, in regards to creating appropriate mock data to assert.

Assignee

Comment 7

4 months ago

Hey James! Added what I think will be the solution to this. However, I was stumped on what data to use to test it in the new location. I imagine I will need to test the methods and properties of the CaptureIO class? But am not sure how to mock data to do so. Could you please point me in the right direction? Thanks! Hopefully everything else looks ok so far!

So it seems there are some failing tests here. The Python 3 failures seem to be because StringIO doesn't exist; I think we can use BytesIO in both cases. The Python 2 failures look like things not working as expected on some platforms; possibly existing bugs in the code, and require more investigation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=693fa6df3832ef2d0145fd1d35a68184ee1c878f is a try push with the StringIO to BytesIO conversion. It seems that there are some more Python 3 issues to look at, plus some Python 2 failures on non-linux platforms.

Assignee

Comment 11

4 months ago

Thanks for looking into it! This is my first time using Treeherder. How do I tell what other Python2/3 discrepancies need fixing? I'm not quite sure what to change here...
Thanks!

Comment 12

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee: nobody → nikkisharpley
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15880 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/15880
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/dCwkAEu2Tf6xxywdt_wDZg)
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/15880
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/SJKQhLUeSSW68iWxsHPCTg)
Upstream PR merged
Upstream PR merged
Upstream PR merged
Upstream PR merged
Upstream PR merged

Comment 22

6 days ago
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09802bf2857c
[wpt PR 15880] - [Gecko Bug 1021926] mozlog: move the capture io class from web-platform/tests to mozlog (bug 1021926), a=testonly
You need to log in before you can comment on or make changes to this bug.