1033568 - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_reorder_buttons

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joshua Cranmer [:jcranmer]]

Effectively, dragging and dropping the widgets to customize the toolbar is broken. The regression was confirmed via a rather annoying hg bisect.

[Filed in such a way so that I can star the several broken mozmill tests]

Regressed by bug 1000514, which I can't mark as blocking (?)

Comment 1

[Joshua Cranmer [:jcranmer]]

Correction: my inability to customize was related to something in the profile; a fresh profile could customize the window.

So it's really just a mozmill synthesized dnd issue.

Comment 2

[:Gijs (he/him)]

I can look at this, although if mconley already knows what's up then that might be quicker.

I expect we can probably just fake the property from the test? But maybe I'm too optimistic.

Comment 3

[Joshua Cranmer [:jcranmer]]

The patch in the other bug is on track to port to m-a and m-b, so I think we'll need to get the eventual patch here tracked as well.

Comment 4

[:Gijs (he/him)]

I can't get mozmill tests to run locally (they open Thunderbird and then just sit there, with a bunch of errors in the console), but looking at the code, I suspect this will either need a similar pref solution as the Firefox tests did, or we need to seriously rearch the mozmill dnd tests...

Can someone point me to an example thunderbird mozmill test that toggles a pref for the purposes of the test?

Comment 5

[Joshua Cranmer [:jcranmer]]

<http://dxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js#240>

Comment 6

[:Gijs (he/him)]

Attachment: add pref for customizeToolbars security check,

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8449829 [details] [diff] [review]
add pref for customizeToolbars security check,

Since this is testing-only and we don't expect QA engineers or anyone else to manually flip the pref, I don't think this should be added to all.js. Instead, just wrap the three lines added to isUnwantedDragEvent in try/catch, and add a blank line after that while you're at it.

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8449829 [details] [diff] [review]
> add pref for customizeToolbars security check,
> 
> Since this is testing-only and we don't expect QA engineers or anyone else
> to manually flip the pref, I don't think this should be added to all.js.
> Instead, just wrap the three lines added to isUnwantedDragEvent in
> try/catch, and add a blank line after that while you're at it.

Done.

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

Comment 9

[:Gijs (he/him)]

Attachment: adjust pref for test,

This should then do. Can you test this, seeing as I can't get mozmill tests to run at all?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8450185 [details] [diff] [review]
adjust pref for test,

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

Looks good - I think we'll want to add something similar to this test too: https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/tabmail/test-tabmail-customize.js#56

Thanks!

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/comm-central/rev/d2c28df60dab

This should be fixed. Let's hang on until we have green (or at least, these failures are gone) on tbpl.

Comment 13

[:Gijs (he/him)]

This is still broken. I don't know why. :-\

Comment 14

[:Gijs (he/him)]

Attachment: include pref in more tests,

Oh, wait, yes I do. There are more of these. Ugh. Sorry for not noticing this first time 'round. Judging by http://mxr.mozilla.org/comm-central/search?string=drag_n_drop&find=mozmill this should be everything.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8450653 [details] [diff] [review]
include pref in more tests,

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

Thanks Gijs!

Comment 16

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/comm-central/rev/0ae1f056a7a5

Comment 17

[:Gijs (he/him)]

3/4 linux builds and all windows XP builds are green, so I think it's safe to say this is now fixed. I'll ask for uplift next.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8449829 [details] [diff] [review]
add pref for customizeToolbars security check,

Approval Request Comment
[Feature/regressing bug #]: bug 1000514
[User impact if declined]: broken thunderbird tests
[Describe test coverage new/current, TBPL]: those thunderbird tests
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 19

[:Gijs (he/him)]

Attachment: Folded patch for comm-aurora/beta

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8450951 [details] [diff] [review]
Folded patch for comm-aurora/beta

a=Standard8 for landing these on comm-aurora/beta (I can't approve the mozilla ones)

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Gijs, can you explain how this impacts Firefox? Thanks

Comment 22

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #21)
> Gijs, can you explain how this impacts Firefox? Thanks

It does not (except potentially for add-ons), but it impacts other toolkit consumers, and we've uplifted the original bug, which also doesn't impact Firefox (except potentially for add-ons). So we should uplift this as well. All it does is make some code that we added listen for a preference, which we can set in tests so that the tests of the respective toolkit consumers don't break.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8449829 [details] [diff] [review]
add pref for customizeToolbars security check,

OK. Taking because 31 is an ESR release.

Comment 24

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/62c43665c993
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/73ce1f980083

Comment 25

[:Gijs (he/him)]

Mark said he could uplift to comm-a/b per IRC discussion; needinfo'ing so this doesn't fall through the cracks :-)

Comment 26

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/1041f0fd1498
https://hg.mozilla.org/releases/comm-beta/rev/345e72f40b3d