Closed
Bug 1338528
Opened 7 years ago
Closed 7 years ago
[mozlog] pytest-mozlog fails when tests are organised within classes
Categories
(Testing :: Mozbase, defect)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: davehunt, Assigned: davehunt, Mentored)
References
Details
Attachments
(3 files)
It's not necessary for pytest tests to be class methods, but it is supported. When attempting to use the pytest-mozlog plugin for tests that are organised within classes, the exception "ValueError: too many values to unpack" occurs as shown in the attachment. This is due to an expectation that the nodeid will only have one occurance of '::' and can be used to split this into a filename and test name. When classes are used, the separator is used between the class and method name. A simple solution would be to use partition to separate the filename from the remaining test identifier.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8836730 [details] Bug 1338528 - [mozlog] Remove formatting of node ID to allow classes and reduce chance of duplicates. https://reviewboard.mozilla.org/r/112078/#review113326 I had actually just ran into this the other day, but then realized that I was doing something wrong and didn't end up needing the fix after all. But I think I had fixed it by doing something like: ids = nodeid.split("::") return ".".join([os.path.basename(ids[0])] + ids[1:]) But I think just removing this is fine as well. It's basically just what do we prefer as a separator right?
Attachment #8836730 -
Flags: review?(ahalberstadt) → review+
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836730 [details] Bug 1338528 - [mozlog] Remove formatting of node ID to allow classes and reduce chance of duplicates. https://reviewboard.mozilla.org/r/112078/#review113326 One thing I just thought of.. Will this change the test ids in the Mn logs? If so, this change will have sheriffing/starring implications.
Assignee | ||
Comment 4•7 years ago
|
||
> But I think just removing this is fine as well. It's basically just what do we prefer as a separator right? It's that, but it's also avoiding the truncation of the the test paths. If we had tests/desktop/test_smoke.py::test_login and tests/mobile/test_smoke.py::test_login for example then the current code would create duplicate test names of "test_smoke.py test_login". > One thing I just thought of.. Will this change the test ids in the Mn logs? If so, this change will have sheriffing/starring implications. More than likely. Who should I notify of this? I believe it's only the Marionette harness tests using this at the moment, but I could be wrong.
Flags: needinfo?(ahalberstadt)
Comment 5•7 years ago
|
||
Ah, if it's just the harness tests then it's probably not a big deal. I was more worried about the actual Mn tests, but I guess those aren't using pytest at all. It's probably safe to just land this then.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 6•7 years ago
|
||
I'm finding myself once again staring at TryChooser and wondering what syntax I should use... I can't see the Marionette harness tests listed or mozbase. Could you perhaps suggest a syntax? It's always so long between my m-c patches that I doubt my notes and my memory, and feel that simply running all the things would be a bad idea.
Flags: needinfo?(ahalberstadt)
Comment 7•7 years ago
|
||
I use -b do -p linux64,macosx64,win64,android-api-15 -u xpcshell,marionette,marionette-e10s,web-platform-tests,firefox-ui-functional,firefox-ui-functional-e10s,external-media-tests,reftest,mochitest -t none to run all relevant Marionette tests. I don’t know which of them are directly related to Mn-h, unfortunately.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
I just noticed by pushing a patch for another bug that I missed removing where the format method was called... Please could you review again? I've also now triggered a try run.
Flags: needinfo?(ahalberstadt)
Comment 10•7 years ago
|
||
The marionette harness tests can't be easily run with try syntax.. Instead they run whenever you modify one of these files: https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/marionette-harness/kind.yml#47 The pytest-mozlog plugin should probably be added to that list, so that when we modify it, we will automatically run the marionette harness tests. I think running that previous try syntax won't do any useful testing.
Comment 11•7 years ago
|
||
Also, I re-reviewed.. though you may want to run the marionette-harness tests on try first. To do that you can: 1. Modify any file under testing/marionette/harness (e.g a README or something) and commit it 2. Run `hg push try` (without any try syntax) For bonus points, you can add testing/mozbase/mozlog/mozlog/pytest_mozlog/** to that list in the kind.yml. If you do that in a new commit, you won't need to do step 1 (I'll also rubber stamp that change if you want to land it).
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a03f1f81b3b
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8836913 [details] Bug 1338528 - Run the Marionette harness tests when pytest-mozlog is updated. https://reviewboard.mozilla.org/r/112228/#review113552
Attachment #8836913 -
Flags: review?(ahalberstadt) → review+
Comment 15•7 years ago
|
||
Pushed by dhunt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/86ea69ac22d5 [mozlog] Remove formatting of node ID to allow classes and reduce chance of duplicates. r=ahal https://hg.mozilla.org/integration/autoland/rev/7fa607a254c0 Run the Marionette harness tests when pytest-mozlog is updated. r=ahal
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86ea69ac22d5 https://hg.mozilla.org/mozilla-central/rev/7fa607a254c0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•