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

RESOLVED FIXED in Firefox 54


2 years ago
2 years ago


(Reporter: gbrown, Assigned: gbrown)



Firefox Tracking Flags

(firefox54 fixed)



(2 attachments)



2 years 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.

This is OK because it has no trailing comment.

subsuite=bar,foo=="bar" # this has a comment
This is OK because it has a condition.

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.

Comment 1

2 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.

Comment 3

2 years ago
(In reply to Geoff Brown [:gbrown] from comment #2)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=791766dc9d0f23a1b6fbe8ba1d249c61d27a213b


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


Comment 4

2 years ago
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

2 years ago
Pushed by gbrown@mozilla.com:
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:

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.

Comment 10

2 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?
I did a quick test, and manifestparser indeed does not support inline comments:

$ cat foobar.ini
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:

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

Comment 13

2 years ago
Thanks :ahal. Let's go ahead here with this change and clean it all up properly in bug 1333564.
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)

Comment 15

2 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+
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

2 years ago
Pushed by ahalberstadt@mozilla.com:
Remove inline comment in manifestparser because they are not supported, r=gbrown
Backout was here:

Sorry for backing out, I understand your argument. I'll try to take a look at bug 1333564 sometime soon.

Comment 19

2 years ago
Last Resolved: 2 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.