Closed Bug 924463 Opened 7 years ago Closed 7 years ago

Clean up browser/components moz.build test manifest specifications

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 920185
Status: NEW → ASSIGNED
Apparently I need the explicit path to tabview's component. New try push: https://tbpl.mozilla.org/?tree=Try&rev=f1eef241842b
Attachment #814465 - Attachment is obsolete: true
Attachment #816526 - Attachment is obsolete: true
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.
Attachment #816813 - Flags: review?(gps)
Attachment #816813 - Flags: review?(gavin.sharp)
Attachment #816632 - Attachment is obsolete: true
Ugh. Splinter did a horrible job of rendering this patch. So, I uploaded it to Phabricator:

http://phabricator.gregoryszorc.com/D3
(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 on attachment 816813 [details] [diff] [review]
clean up browser/components moz.build/test manifest,

I don't think I need to review this!
Attachment #816813 - Flags: review?(gavin.sharp)
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.
Attachment #816813 - Flags: review?(gps) → review+
(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!
Landed with comments addressed: https://hg.mozilla.org/integration/fx-team/rev/e9f46c81c28e
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e9f46c81c28e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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...).
(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). :-|
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.
Status: REOPENED → ASSIGNED
Please test this on the try server by retriggering mochitest-bc runs a bunch of times before relanding.
(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?
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?
(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.
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
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4cec7d72e003
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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.
You need to log in before you can comment on or make changes to this bug.