924463 - Clean up browser/components moz.build test manifest specifications

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[:Gijs (he/him)]

Attachment: clean up browser/components moz.build/test manifest,

I believe this works. Try build: https://tbpl.mozilla.org/?tree=Try&rev=47ce6f051b81

Comment 2

[:Gijs (he/him)]

Attachment: clean up browser/components moz.build/test manifest,

Apparently I need the explicit path to tabview's component. New try push: https://tbpl.mozilla.org/?tree=Try&rev=f1eef241842b

Comment 3

[:Gijs (he/him)]

Attachment: clean up browser/components moz.build/test manifest,

Third time lucky? https://tbpl.mozilla.org/?tree=Try&rev=4933b7d27090

Comment 4

[:Gijs (he/him)]

Attachment: clean up browser/components moz.build/test manifest,

So the syntax for skip-if does actually include quotes, unlike the MDN wikipage says. The try run was green apart from these tests which ought to be disabled on Mac anyway, so going for review.

Comment 5

[Gregory Szorc [:gps]]

Ugh. Splinter did a horrible job of rendering this patch. So, I uploaded it to Phabricator:

http://phabricator.gregoryszorc.com/D3

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Created attachment 816813 [details] [diff] [review]
> clean up browser/components moz.build/test manifest,
> 
> So the syntax for skip-if does actually include quotes, unlike the MDN
> wikipage says. The try run was green apart from these tests which ought to
> be disabled on Mac anyway, so going for review.

I filed bug 926821 about the manifest syntax confusion wrt skip-if conditions.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 816813 [details] [diff] [review]
clean up browser/components moz.build/test manifest,

I don't think I need to review this!

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 816813 [details] [diff] [review]
clean up browser/components moz.build/test manifest,

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

r+ if you move the manifests back to their children.

::: browser/components/moz.build
@@ +54,5 @@
> +    'search/test/browser.ini',
> +    'sessionstore/test/browser.ini',
> +    'shell/test/browser.ini',
> +    'tabview/test/browser.ini',
> +    'test/browser.ini'

I'm not a huge fan of moving test manifest declaration beyond intermediate moz.build files. Please keep manifest declaration in leaf moz.build files if possible.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gregory Szorc [:gps] from comment #8)
> I'm not a huge fan of moving test manifest declaration beyond intermediate
> moz.build files. Please keep manifest declaration in leaf moz.build files if
> possible.

I concur!

Comment 10

[:Gijs (he/him)]

Landed with comments addressed: https://hg.mozilla.org/integration/fx-team/rev/e9f46c81c28e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9f46c81c28e

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/e25e62d174ed - once we finally got enough retriggers to run, this turned out to be the cause of the very frequent (on Linux and especially on debug 10.7 and 10.8 Mac, but not on Windows because the test has been disabled there for a long time) https://tbpl.mozilla.org/php/getParsedLog.php?id=29341166&tree=Fx-Team failures.

Comment 13

[:Gijs (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #12)
> Backed out in https://hg.mozilla.org/mozilla-central/rev/e25e62d174ed - once
> we finally got enough retriggers to run, this turned out to be the cause of
> the very frequent (on Linux and especially on debug 10.7 and 10.8 Mac, but
> not on Windows because the test has been disabled there for a long time)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=29341166&tree=Fx-Team
> failures.

Well, that sucked. Sorry! And thank you for backing me out.

Looking at this, I... actually have 0 clue what's going on. At first I thought that maybe these were meant to be non-browser mochitests (because "MOCHITEST_FILES" rather than "MOCHITEST_BROWSER_FILES" in the makefile) despite their "browser_" prefixes, but they're running as browser mochitests now, too. If I messed up the manifest in a way that the support files were missing (e.g. because I put a comment in the middle of the sequence of support files) then I'd expect it to fail 100% of the time, not "sometimes". Even the ordering in the test manifest is the same as that in the makefile. So, err, I'm a bit lost as to how this magically started breaking.

I don't really have time to look into this in more detail today. If I find time tomorrow, I'll probably reland this without the sessionstore portion, and file a followup bug for that to investigate these failures there (and do still more try runs...).

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Phil Ringnalda (:philor) from comment #12)
> > Backed out in https://hg.mozilla.org/mozilla-central/rev/e25e62d174ed - once
> > we finally got enough retriggers to run, this turned out to be the cause of
> > the very frequent (on Linux and especially on debug 10.7 and 10.8 Mac, but
> > not on Windows because the test has been disabled there for a long time)
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=29341166&tree=Fx-Team
> > failures.
> 
> Well, that sucked. Sorry! And thank you for backing me out.
> 
> Looking at this, I... actually have 0 clue what's going on. At first I
> thought that maybe these were meant to be non-browser mochitests (because
> "MOCHITEST_FILES" rather than "MOCHITEST_BROWSER_FILES" in the makefile)
> despite their "browser_" prefixes, but they're running as browser mochitests
> now, too.

Err, actually, I'm now struggling to find *any* logfile where they show up on the cset after the backout (ie they're in neither mochitest 1-5 nor mochitest-bc, at least on OS X 10.8 debug where I just checked). :-|

Comment 15

[:Gijs (he/him)]

OK, so these were originally browser mochitests, but then when bug 552424 tried to disable them on Windows, it (presumably inadvertently) disabled them *everywhere* by putting them in MOCHITEST_FILES instead of MOCHITEST_BROWSER_FILES, and by now they've clearly broken on more platforms. I'll reland with skip-if = true when I have some time, and file a bug on fixing the test when I do that.

Comment 16

[(no longer active)]

Please test this on the try server by retriggering mochitest-bc runs a bunch of times before relanding.

Comment 17

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> Please test this on the try server by retriggering mochitest-bc runs a bunch
> of times before relanding.

Why? The only thing that broke were the tests that I will be disabling completely (because they were disabled completely before). Is there other bustage that I missed?

Comment 18

[Phil Ringnalda (:philor)]

No.

That was some great dancing-in-clownshoes on our part, but I'd rather not see it again, I want those test files out of the tree. Would it be easier for you to rebase around if you do the hg rm of them, or should I?

Comment 19

[:Gijs (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #18)
> No.
> 
> That was some great dancing-in-clownshoes on our part, but I'd rather not
> see it again, I want those test files out of the tree. Would it be easier
> for you to rebase around if you do the hg rm of them, or should I?

I don't understand why we should be hg rm'ing them rather than leaving them disabled (properly this time!) and file a bug for someone to fix them. Seems like that'll be just as good at keeping the tree green. Anyway, I'm also not the right person to be debating that. Tim should at least know about it, as he more or less owns sessionstore these days, from what I've been able to tell (TL;DR for Tim: some old tests accidentally got totally disabled when they were meant to only be disabled on Windows, and started failing intermittently when I migrated them and properly only disabled them on Windows, and Phil is proposing we hg rm the relevant tests). I'll look at this again tomorrow night (Europe time) or Monday, and if the files are still there I'll disable them, otherwise I'll just update the patch to not mention them at all.

Comment 20

[:Gijs (he/him)]

These files seem to have survived the past 24-odd-h, so I've pushed with them disabled:

https://hg.mozilla.org/integration/fx-team/rev/4cec7d72e003

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Drew Willcoxon :adw]

FYI, this broke the preprocessed tests in browser/components/search/test/.  See bug 975528 comment 15.  You might want to make sure it didn't break other tests.