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)

Version 3
defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: davehunt, Assigned: davehunt, Mentored)

References

Details

Attachments

(3 files)

Attached file exception.txt
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.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
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 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.
> 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)
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)
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)
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.
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)
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.
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).
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+
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
https://hg.mozilla.org/mozilla-central/rev/86ea69ac22d5
https://hg.mozilla.org/mozilla-central/rev/7fa607a254c0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1339462
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: