Closed
Bug 1333506
Opened 7 years ago
Closed 7 years ago
Test is not run if manifest entry has 'subsuite' with comment
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: gbrown, Assigned: gbrown)
References
Details
Attachments
(2 files)
3.12 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
If a test manifest has an entry with a subsuite, and the subsuite line has a trailing comment, and no conditions, the test fails to run in any suite. [test1] subsuite=bar ^^^ This is OK because it has no trailing comment. [test1] subsuite=bar,foo=="bar" # this has a comment ^^^ This is OK because it has a condition. [test1] subsuite=bar # this has a comment ^^^ This probably assigns the test to subsuite "bar # this has a comment", which doesn't exist, so the test never runs.
Assignee | ||
Comment 1•7 years ago
|
||
Practically, this appears to only affect browser_newtab_drag_drop_ext.js, in browser/base/content/test/newtab/browser.ini; it should run in mochitest-clipboard on non-Linux platforms, but does not.
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=791766dc9d0f23a1b6fbe8ba1d249c61d27a213b
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Geoff Brown [:gbrown] from comment #2) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=791766dc9d0f23a1b6fbe8ba1d249c61d27a213b https://archive.mozilla.org/pub/firefox/try-builds/gbrown@mozilla.com-791766dc9d0f23a1b6fbe8ba1d249c61d27a213b/try-win32/try_win7_ix_test-mochitest-clipboard-e10s-bm127-tests1-windows-build1341.txt.gz 11:46:41 INFO - TEST-START | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js 11:46:49 INFO - MEMORY STAT | vsize 581MB | vsizeMaxContiguous 514MB | residentFast 231MB | heapAllocated 88MB 11:46:49 INFO - TEST-OK | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | took 7596ms Excellent!
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8829991 -
Flags: review?(jmaher)
Comment 5•7 years ago
|
||
Comment on attachment 8829991 [details] [diff] [review] allow trailing comments on subsuite lines Review of attachment 8829991 [details] [diff] [review]: ----------------------------------------------------------------- thanks for fixing this!
Attachment #8829991 -
Flags: review?(jmaher) → review+
Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f48f9edd42 Allow trailing comment after subsuite in test manifest; r=jmaher
Comment 7•7 years ago
|
||
I'm a little confused.. I was pretty sure inline comments weren't allowed by manifestparser anywhere, and the docs seem to support this: http://mozbase.readthedocs.io/en/latest/manifestparser.html#manifest-format Did this feature get added somewhere and the docs just didn't get updated?
Comment 8•7 years ago
|
||
So, the fix here probably should have been in ini.py, not filters.py. If we support inline comments (which I don't think we do), then the comment should have been stripped off long before we ever got to filters.py.
Comment 9•7 years ago
|
||
looking at *.ini there are dozens of inline comments on the skip-if lines, so the cat is out of the bag.
Assignee | ||
Comment 10•7 years ago
|
||
Right, inline comments are routine for skip-if's. Does that just happen to work? If we do not support inline comments, we need to do something to reject them / error out when they are encountered. If we support inline comments in some contexts (like skip-if), then we should support them everywhere. I'm not very familiar with manifestparser; ahal, do you want to sort this out?
Comment 11•7 years ago
|
||
I did a quick test, and manifestparser indeed does not support inline comments: $ cat foobar.ini [foo] bar=baz # fleem $ python >>> mp = TestManifest >>> mp.read('foobar.ini') >>> mp.tests[0] { 'bar': 'baz # fleem', ... } The reason it works for skip-if and subsuite-if, is because we are stripping the comment off when the expression gets evaluated here: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/ini.py#141 I don't know why this behaviour was added, and I think this is a bad situation. We should either support inline comments everywhere, or nowhere (I'd prefer everywhere). But either way, we shouldn't special case the fix just for the 'subsuite' key when inline comments don't work anywhere else either (except for conditional keys). I guess we can leave this patch in for now, as long as we file a follow-up to remove it and support inline comments everywhere properly (though my mild preference would be to back it out).
Comment 12•7 years ago
|
||
Also the current solution will fail on edge cases, which granted, won't ever show up in practice.. But e.g, I wouldn't expect: subsuite = "foo#bar" to be treated as a comment. I think jhammel purposefully didn't support inline comments just for this reason.
Assignee | ||
Comment 13•7 years ago
|
||
Thanks :ahal. Let's go ahead here with this change and clean it all up properly in bug 1333564.
Comment 14•7 years ago
|
||
Actually, sorry Geoff.. do you mind if I backout your change and land this one instead? I agree manifestparser's current behaviour is really dumb, and I filed bug 1333564 to fix it, but I'm not sure how easy it will be to fix properly. In the meantime I think it would be better to just remove the single inline comment that was hitting this.
Attachment #8830040 -
Flags: review?(gbrown)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8830040 [details] [diff] [review] Remove inline comment on subsuite key Review of attachment 8830040 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that's the best way forward. Someone (maybe even me) is going to make the same mistake and accidentally, silently skip a test. Inline comments are natural, and partially supported, even without my change. But this is more your area than mine, so, reluctantly, r+.
Attachment #8830040 -
Flags: review?(gbrown) → review+
Comment 16•7 years ago
|
||
From my point of view, the default behaviour is that inline comments are not supported. Then there are some keys that are special cased so that they are supported (they should never have been special cased in the first place). Then this patch special cases another key in a different way than the others. My worry is that even more keys will start being special cased in different ways in the future. Then it will be impossible to know how (or whether) inline comments will work for any given key. Imo this would be even more confusing than the current situation.
Comment 17•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c554725ceaa Remove inline comment in manifestparser because they are not supported, r=gbrown
Comment 18•7 years ago
|
||
Backout was here: https://hg.mozilla.org/integration/mozilla-inbound/rev/66e3e46608deb6279700cd7a018d53371f903339 Sorry for backing out, I understand your argument. I'll try to take a look at bug 1333564 sometime soon.
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c554725ceaa
Status: NEW → 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
•