If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Test is not run if manifest entry has 'subsuite' with comment

RESOLVED FIXED in Firefox 54

Status

Testing
Mozbase
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
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

9 months 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

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=791766dc9d0f23a1b6fbe8ba1d249c61d27a213b
(Assignee)

Comment 3

9 months 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

9 months ago
Created attachment 8829991 [details] [diff] [review]
allow trailing comments on subsuite lines
Attachment #8829991 - Flags: review?(jmaher)
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+

Comment 6

9 months ago
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f48f9edd42
Allow trailing comment after subsuite in test manifest; r=jmaher
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?
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.
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

9 months 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?
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).
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.
See Also: → bug 1333564
(Assignee)

Comment 13

9 months ago
Thanks :ahal. Let's go ahead here with this change and clean it all up properly in bug 1333564.
Created attachment 8830040 [details] [diff] [review]
Remove inline comment on subsuite key

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

9 months 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+
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

9 months 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
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c554725ceaa
Status: NEW → RESOLVED
Last Resolved: 9 months 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.