Closed
Bug 1093507
Opened 10 years ago
Closed 10 years ago
Permaorange on Gum in browser_876944_customize_mode_create_destroy.js - "The number of placeholders should be correct"
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox35 fixed, firefox36 fixed)
RESOLVED
FIXED
Firefox 36
People
(Reporter: jwalker, Assigned: ejpbruel)
References
Details
Attachments
(1 file)
2.24 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
Comment 1•10 years ago
|
||
My main suspect is bug 1067337, but Eddy is bisecting now and should have results soon.
Assignee | ||
Comment 2•10 years ago
|
||
The first bad revision is: changeset: 225871:6994889d019e tag: tip user: Victor Porof <vporof@mozilla.com> date: Mon Oct 27 12:26:07 2014 -0400 summary: Bug 1067337 - Provide an icon for the toolbar to activate/deactivate the devtools panel, r=paul,gijs
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 18•10 years ago
|
||
Boo!
Comment 19•10 years ago
|
||
This test wasn't failing when I pushed to try. Looks like something changed in the meantime.
Comment 20•10 years ago
|
||
The difference is that on Gum MOZ_DEV_EDITION is defined.
Comment 21•10 years ago
|
||
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #20) > The difference is that on Gum MOZ_DEV_EDITION is defined. That's even weirder. Why would this pass locally then? Anyway, I'm taking this.
Updated•10 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Comment 22•10 years ago
|
||
Make sure you are building locally with this in your mozconfig: ac_add_options --enable-update-channel=nightly-gum Or even with s/nightly-gum/aurora/.
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #21) > (In reply to Panos Astithas [:past] (overloaded, please needinfo) from > comment #20) > > The difference is that on Gum MOZ_DEV_EDITION is defined. > > That's even weirder. Why would this pass locally then? Anyway, I'm taking > this. Thanks for taking this. Would you be so kind to ping me on irc when you're about to land the fix on gum?
Assignee | ||
Comment 24•10 years ago
|
||
I managed to reproduce the test failure locally with the mozconfig line from comment 22. I've pushed the following patch to gum: https://hg.mozilla.org/projects/gum/rev/527e37564f9d This should take care of the problem. I only tested locally, but since the patch only affects the test, it should be safe to skip testing the patch on try.
Comment 25•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #24) > I managed to reproduce the test failure locally with the mozconfig line from > comment 22. I've pushed the following patch to gum: > https://hg.mozilla.org/projects/gum/rev/527e37564f9d > > This should take care of the problem. I only tested locally, but since the > patch only affects the test, it should be safe to skip testing the patch on > try. So essentially, this failed on gum, didn't fail on fx-team because no DEVEDITION thing is defined... won't this fail on aurora when we do define that thing there? Shouldn't we push the same change to fx-team and ensure it gets uplifted, too?
Flags: needinfo?(ejpbruel)
Comment 26•10 years ago
|
||
This very similar to the fix I was going to land, as per the changes I made in a similar test for bug 1067337: https://hg.mozilla.org/mozilla-central/rev/f3cc40b3abf2#l5.199 Looks like Eddy beat me to it :)
Assignee: vporof → ejpbruel
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #25) > (In reply to Eddy Bruel [:ejpbruel] from comment #24) > > I managed to reproduce the test failure locally with the mozconfig line from > > comment 22. I've pushed the following patch to gum: > > https://hg.mozilla.org/projects/gum/rev/527e37564f9d > > > > This should take care of the problem. I only tested locally, but since the > > patch only affects the test, it should be safe to skip testing the patch on > > try. > > So essentially, this failed on gum, didn't fail on fx-team because no > DEVEDITION thing is defined... won't this fail on aurora when we do define > that thing there? Shouldn't we push the same change to fx-team and ensure it > gets uplifted, too? The plan is to eventually merge gum back into aurora, in which case this problem should resolve itself, no?
Flags: needinfo?(ejpbruel)
Comment 28•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #27) > (In reply to :Gijs Kruitbosch from comment #25) > > (In reply to Eddy Bruel [:ejpbruel] from comment #24) > > > I managed to reproduce the test failure locally with the mozconfig line from > > > comment 22. I've pushed the following patch to gum: > > > https://hg.mozilla.org/projects/gum/rev/527e37564f9d > > > > > > This should take care of the problem. I only tested locally, but since the > > > patch only affects the test, it should be safe to skip testing the patch on > > > try. > > > > So essentially, this failed on gum, didn't fail on fx-team because no > > DEVEDITION thing is defined... won't this fail on aurora when we do define > > that thing there? Shouldn't we push the same change to fx-team and ensure it > > gets uplifted, too? > > The plan is to eventually merge gum back into aurora, in which case this > problem should resolve itself, no? Either way, this still needs to land on m-c.
Comment 29•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #27) > (In reply to :Gijs Kruitbosch from comment #25) > > (In reply to Eddy Bruel [:ejpbruel] from comment #24) > > > I managed to reproduce the test failure locally with the mozconfig line from > > > comment 22. I've pushed the following patch to gum: > > > https://hg.mozilla.org/projects/gum/rev/527e37564f9d > > > > > > This should take care of the problem. I only tested locally, but since the > > > patch only affects the test, it should be safe to skip testing the patch on > > > try. > > > > So essentially, this failed on gum, didn't fail on fx-team because no > > DEVEDITION thing is defined... won't this fail on aurora when we do define > > that thing there? Shouldn't we push the same change to fx-team and ensure it > > gets uplifted, too? > > The plan is to eventually merge gum back into aurora, in which case this > problem should resolve itself, no? What Brian said. But also, I asked this only last week and was specifically told this was not the plan, so I'm now very confused. Also, and I'm sorry for the impression of stop energy / bad cop here... but ideally the test change should get review. :-) *looks* Consider this comment r+ from me, but please factor out the expression for the expected number of placeholders into a variable (like, say, "expectedPlaceholders") and use that in the comparison.
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #29) > (In reply to Eddy Bruel [:ejpbruel] from comment #27) > > (In reply to :Gijs Kruitbosch from comment #25) > > > (In reply to Eddy Bruel [:ejpbruel] from comment #24) > > > > I managed to reproduce the test failure locally with the mozconfig line from > > > > comment 22. I've pushed the following patch to gum: > > > > https://hg.mozilla.org/projects/gum/rev/527e37564f9d > > > > > > > > This should take care of the problem. I only tested locally, but since the > > > > patch only affects the test, it should be safe to skip testing the patch on > > > > try. > > > > > > So essentially, this failed on gum, didn't fail on fx-team because no > > > DEVEDITION thing is defined... won't this fail on aurora when we do define > > > that thing there? Shouldn't we push the same change to fx-team and ensure it > > > gets uplifted, too? > > > > The plan is to eventually merge gum back into aurora, in which case this > > problem should resolve itself, no? > > What Brian said. But also, I asked this only last week and was specifically > told this was not the plan, so I'm now very confused. > > Also, and I'm sorry for the impression of stop energy / bad cop here... but > ideally the test change should get review. :-) > > *looks* > > Consider this comment r+ from me, but please factor out the expression for > the expected number of placeholders into a variable (like, say, > "expectedPlaceholders") and use that in the comparison. Ugh. No offense, but complaining about style nits in a 2 line fix in a test *does* sound like spending energy on the wrong things. I already pushed the patch to gum, so if you really feel strongly about this, I can cook up another patch that fixes the style nits, push that to gum, and then push both patches to m-c tomorrow.
Comment 31•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #30) > Ugh. No offense, but complaining about style nits in a 2 line fix in a test > *does* sound like spending energy on the wrong things. Landing non-obvious test changes without review is something I think we should avoid; that the result of the review was just style nits is orthogonal to that. > I already pushed the patch to gum, so if you really feel strongly about > this, I can cook up another patch that fixes the style nits, push that to > gum, and then push both patches to m-c tomorrow. As far as I'm concerned, there's no need to "fix" gum for the style nits considering it's already landed. But for what you land on fx-team, I don't think fixing it is that much to ask - it's essentially what would have happened if you attached a patch and requested review before landing! I'd still love to have more clarity on whether or not we intend to merge gum to aurora.
Comment 32•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #31) > (In reply to Eddy Bruel [:ejpbruel] from comment #30) > > Ugh. No offense, but complaining about style nits in a 2 line fix in a test > > *does* sound like spending energy on the wrong things. > > Landing non-obvious test changes without review is something I think we > should avoid; that the result of the review was just style nits is > orthogonal to that. > > > I already pushed the patch to gum, so if you really feel strongly about > > this, I can cook up another patch that fixes the style nits, push that to > > gum, and then push both patches to m-c tomorrow. > > As far as I'm concerned, there's no need to "fix" gum for the style nits > considering it's already landed. But for what you land on fx-team, I don't > think fixing it is that much to ask - it's essentially what would have > happened if you attached a patch and requested review before landing! > > > I'd still love to have more clarity on whether or not we intend to merge gum > to aurora. That is the plan, so we do need to make sure things that stick there are reviewed. The reason for landing this on Gum immediately was to confirm that it fixed the tests before asking for review. And it was a simple enough patch that I assumed we could back it out and reland if it needed changes. This made sense to me at the time but as I think about it more, I wonder if we could just use Try from a local Gum build. This would get the same effect without needing to use Gum as a test server. Here is a sample push with nothing else in the queue: https://tbpl.mozilla.org/?tree=Try&rev=8bcd65367907, and one with the patch that landed on gum applied: https://tbpl.mozilla.org/?tree=Try&rev=01653f6fc82b. But I'm not completely sure if that's working as I hope, since the second push doesn't show the original changesets.
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #31) > (In reply to Eddy Bruel [:ejpbruel] from comment #30) > > Ugh. No offense, but complaining about style nits in a 2 line fix in a test > > *does* sound like spending energy on the wrong things. > > Landing non-obvious test changes without review is something I think we > should avoid; that the result of the review was just style nits is > orthogonal to that. > Ok. I can agree with that. I felt the change was small enough that I could afford a r=me. In hindsight though, since it's not obvious why the change is needed it might have been better to ask for a proper review. I'll keep that in mind in the future. > > I already pushed the patch to gum, so if you really feel strongly about > > this, I can cook up another patch that fixes the style nits, push that to > > gum, and then push both patches to m-c tomorrow. > > As far as I'm concerned, there's no need to "fix" gum for the style nits > considering it's already landed. But for what you land on fx-team, I don't > think fixing it is that much to ask - it's essentially what would have > happened if you attached a patch and requested review before landing! > That's fair enough. > > I'd still love to have more clarity on whether or not we intend to merge gum > to aurora.
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #32) > (In reply to :Gijs Kruitbosch from comment #31) > > (In reply to Eddy Bruel [:ejpbruel] from comment #30) > > > Ugh. No offense, but complaining about style nits in a 2 line fix in a test > > > *does* sound like spending energy on the wrong things. > > > > Landing non-obvious test changes without review is something I think we > > should avoid; that the result of the review was just style nits is > > orthogonal to that. > > > > > I already pushed the patch to gum, so if you really feel strongly about > > > this, I can cook up another patch that fixes the style nits, push that to > > > gum, and then push both patches to m-c tomorrow. > > > > As far as I'm concerned, there's no need to "fix" gum for the style nits > > considering it's already landed. But for what you land on fx-team, I don't > > think fixing it is that much to ask - it's essentially what would have > > happened if you attached a patch and requested review before landing! > > > > > > I'd still love to have more clarity on whether or not we intend to merge gum > > to aurora. > > That is the plan, so we do need to make sure things that stick there are > reviewed. > > The reason for landing this on Gum immediately was to confirm that it fixed > the tests before asking for review. And it was a simple enough patch that I > assumed we could back it out and reland if it needed changes. This made > sense to me at the time but as I think about it more, I wonder if we could > just use Try from a local Gum build. This would get the same effect without > needing to use Gum as a test server. Here is a sample push with nothing > else in the queue: https://tbpl.mozilla.org/?tree=Try&rev=8bcd65367907, and > one with the patch that landed on gum applied: > https://tbpl.mozilla.org/?tree=Try&rev=01653f6fc82b. But I'm not completely > sure if that's working as I hope, since the second push doesn't show the > original changesets. FWIW, the second push doesn't show the original changeset because you already pushed those to try with your first attempt. TBPL only shows newly introduced commits with each push.
Comment 35•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #29) > But also, I asked this only last week and was specifically > told this was not the plan, so I'm now very confused. I didn't lie last week, there was a change of plans yesterday :) Although if we manage to get every necessary patch in Aurora through the usual route, which is still our regular M.O., we may have nothing left to merge in the end.
Comment 36•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #33) > (In reply to :Gijs Kruitbosch from comment #31) > > (In reply to Eddy Bruel [:ejpbruel] from comment #30) > > > Ugh. No offense, but complaining about style nits in a 2 line fix in a test > > > *does* sound like spending energy on the wrong things. > > > > Landing non-obvious test changes without review is something I think we > > should avoid; that the result of the review was just style nits is > > orthogonal to that. > > > > Ok. I can agree with that. I felt the change was small enough that I could > afford a r=me. In hindsight though, since it's not obvious why the change is > needed it might have been better to ask for a proper review. I'll keep that > in mind in the future. OK, the push looks green - I'm going to proceed with landing other patches on gum, but we should come back tomorrow and backout / reland this in whatever form is going to land on m-c.
Assignee | ||
Comment 37•10 years ago
|
||
I've backed out the fix locally and prepared a new patch, with review comments bij Gijs addressed, that should be able to land on both m-c and gum. Putting it up for review here just in case Gijs wants to take another look at it.
Attachment #8517329 -
Flags: review?(gijskruitbosch+bugs)
Comment 38•10 years ago
|
||
Comment on attachment 8517329 [details] [diff] [review] Fix for permaorange for both m-c and gum Review of attachment 8517329 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8517329 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #38) > Comment on attachment 8517329 [details] [diff] [review] > Fix for permaorange for both m-c and gum > > Review of attachment 8517329 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! You're welcome! I hope I didn't come across as too grumpy yesterday ;-)
Assignee | ||
Comment 40•10 years ago
|
||
Backed out the patch and pushed the new one to gum: https://hg.mozilla.org/projects/gum/rev/290d3da2a59e
Assignee | ||
Comment 41•10 years ago
|
||
Merged m-a with gum: https://hg.mozilla.org/projects/gum/pushloghtml?changeset=639b0f85ca34
Assignee | ||
Comment 42•10 years ago
|
||
Pushed the fix for the permaorange to fx-team: https://hg.mozilla.org/integration/fx-team/rev/34278781da80
https://hg.mozilla.org/mozilla-central/rev/34278781da80
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 44•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/290d3da2a59e
Flags: in-testsuite+
Updated•10 years ago
|
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•