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)

defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: jwalker, Assigned: ejpbruel)

References

Details

Attachments

(1 file)

chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
My main suspect is bug 1067337, but Eddy is bisecting now and should have results soon.
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
Blocks: 1067337
This test wasn't failing when I pushed to try. Looks like something changed in the meantime.
The difference is that on Gum MOZ_DEV_EDITION is defined.
(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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
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/.
(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?
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.
(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)
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
(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)
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
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 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+
(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 ;-)
Backed out the patch and pushed the new one to gum:
https://hg.mozilla.org/projects/gum/rev/290d3da2a59e
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: