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

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

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.

Comment 1

[Geoff Brown [:gbrown] (pto Apr 11-18)]

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 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=791766dc9d0f23a1b6fbe8ba1d249c61d27a213b

Comment 3

[Geoff Brown [:gbrown] (pto Apr 11-18)]

(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/[e-mail]-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!

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: allow trailing comments on subsuite lines

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8829991 [details] [diff] [review]
allow trailing comments on subsuite lines

Review of attachment 8829991 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for fixing this!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f48f9edd42
Allow trailing comment after subsuite in test manifest; r=jmaher

Comment 7

[Andrew Halberstadt [:ahal]]

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

[Andrew Halberstadt [:ahal]]

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

[Joel Maher ( :jmaher ) (UTC -8)]

looking at *.ini there are dozens of inline comments on the skip-if lines, so the cat is out of the bag.

Comment 10

[Geoff Brown [:gbrown] (pto Apr 11-18)]

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

[Andrew Halberstadt [:ahal]]

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

[Andrew Halberstadt [:ahal]]

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.

Comment 13

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Thanks :ahal. Let's go ahead here with this change and clean it all up properly in bug 1333564.

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: 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.

Comment 15

[Geoff Brown [:gbrown] (pto Apr 11-18)]

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

Comment 16

[Andrew Halberstadt [:ahal]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c554725ceaa
Remove inline comment in manifestparser because they are not supported, r=gbrown

Comment 18

[Andrew Halberstadt [:ahal]]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4c554725ceaa