746766 - Landing of Downloads Panel broke Download Manager tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the landing of bug 726444 we will now show a panel instead of the Download Manager. This caused breakage in a couple of Mozmill tests:

/testDownloading/testCloseDownloadManager.js:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=14.0&test=%2FtestDownloading%2FtestCloseDownloadManager.js&func=testCloseDownloadManager.js%3A%3AtestCloseDownloadManager

/testDownloading/testOpenDownloadManager.js:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=14.0&test=%2FtestDownloading%2FtestOpenDownloadManager.js&func=testOpenDownloadManager.js%3A%3AtestOpenDownloadManager

/testPrivateBrowsing/testDownloadManagerClosed.js:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=14.0&test=%2FtestPrivateBrowsing%2FtestDownloadManagerClosed.js&func=testDownloadManagerClosed.js%3A%3AtestDownloadManagerClosed

Anthony, can you please give feedback from the QA side how the correct behavior for those tests would be now? Best would be if the Litmus tests would be updated. If this takes longer for you we should mark those tests as skipped for now.

Assigning Anthony for now for the information retrieval.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Simona, for the above tests we would need information from you what the new behavior is. Please come back to us asap. Thanks.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (skip tests) [checked-in]

As long as we don't know what the expected behavior is, we should better skip those tests.

Comment 3

[u279076]

Comment on attachment 616321 [details] [diff] [review]
Patch v1 (skip tests) [checked-in]

Looks good, please land. Thanks

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 616321 [details] [diff] [review]
Patch v1 (skip tests) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c2fd6abd3655

Anthony, please keep in mind that we should *NOT* let this patch settle down to Aurora. The new UI will only be present in Nightly for now. Means for the merge you will have to be careful.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Setting '[do-not-merge]' whiteboard entry for now so we can easily track patches like those.

Comment 6

[u279076]

The panel-based download manager is not getting enabled when we merge to Aurora? Simona, can you confirm this?

Comment 7

[Simona Badau, Desktop QA]

Yes, I can confirm it. You can find more details in Bug 726444 Comment 96.

Comment 8

[u279076]

(In reply to Simona B [QA] from comment #7)
> Yes, I can confirm it. You can find more details in Bug 726444 Comment 96.

Given that target milestone is Firefox 14, I would say this patch *should* get merged forward to Aurora. When Firefox 14 merges to Aurora next week, so will the Mozmill Test failures. That said, it would be great if we could get these tests refactored before we merge next week.

Vlad, can you see what can be done about this in such short time?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This will *NOT* be enabled in Aurora. Please read the corresponding comment exactly. So as I said above we do not want to merge this into mozilla-aurora.

Comment 10

[u279076]

Oh yeah, you are right. So we should keep this disabled on Aurora until they decide to enable it. Simona, can you keep us notified when it gets enabled?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #10)
> decide to enable it. Simona, can you keep us notified when it gets enabled?

I would be more interested in updated information how we should handle the panel correctly. So we can also re-enable the tests for Nightly.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #8)
> (In reply to Simona B [QA] from comment #7)
> > Yes, I can confirm it. You can find more details in Bug 726444 Comment 96.
> 
> Given that target milestone is Firefox 14, I would say this patch *should*
> get merged forward to Aurora. When Firefox 14 merges to Aurora next week, so
> will the Mozmill Test failures. That said, it would be great if we could get
> these tests refactored before we merge next week.
> 
> Vlad, can you see what can be done about this in such short time?

We have to refactor our tests 
Henrik you are assigned to the bug. Should I go on and take it?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, I don't assign myself just for fun. But you can help here in getting in touch with Simona and figuring out the correct behavior so that I can update the libs and tests.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> No, I don't assign myself just for fun. But you can help here in getting in
> touch with Simona and figuring out the correct behavior so that I can update
> the libs and tests.

I'll get to you asap

Comment 15

[Simona Badau, Desktop QA]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #10)
> Oh yeah, you are right. So we should keep this disabled on Aurora until they
> decide to enable it. Simona, can you keep us notified when it gets enabled?

Sure, I will let you know as soon as the feature is enabled per default in Aurora.

Comment 16

[Simona Badau, Desktop QA]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Simona, for the above tests we would need information from you what the new
> behavior is. Please come back to us asap. Thanks.

/testDownloading/testOpenDownloadManager.js:
For the second test the steps to perform are the same, only the expected results are different - now the Download Manager is not opened as a dialog but as a panel.


/testPrivateBrowsing/testDownloadManagerClosed.js:
Like in the second test the only thing that is different is that the Download Manager is a panel and not a dialog.

/testDownloading/testCloseDownloadManager.js:
I'm not sure if all the STR from the first test are applicable on the new Download Manager panel. For instance currently pressing on the Esc key closes the Download panel, but using the keyboard shortcuts Ctrl+W closes the main window. I will need to investigate this more before giving a right answer.

Paolo, could you please take a look over this testcase and point out to the right behavior?
https://litmus.mozilla.org/show_test.cgi?id=15446

Comment 17

[:Paolo Amadini]

To make the existing tests work the same in Nightly and Aurora, you can enable
the "browser.download.useToolkitUI" preference while testing, like we did for the
Toolkit tests of the old Downloads window (see the patch in bug 726444). That is
the same preference that will be enabled by default in Aurora.

In the end, new tests will need to be written for the Downloads Panel in any case,
but we will keep the tests for the old Downloads window enabled in Toolkit, as
that component is still in use by other products. You might want to keep the old
Downloads window tests as well.

As always, the feature page is the central reference point if you have any doubt:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager

Now there is also a more friendly, shorter status page here:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager/Status

Comment 18

[:Paolo Amadini]

(In reply to Simona B [QA] from comment #16)
> Paolo, could you please take a look over this testcase and point out to the
> right behavior?
> https://litmus.mozilla.org/show_test.cgi?id=15446

That test case is still valid for the old Downloads window, I think?

As for closing the panel, the ESC key and clicking outside the panel's area are
the only methods that I know of. Pressing the shortcut key twice should *not*
close the panel, differently from what happened with the window.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Paolo. I think that makes sense. So for now we will make use of the preference so we can re-enable the tests again. Once we have the new panel available in Aurora and tests added to Litmus we can also add new tests for the panel on another bug.

Simona, also thank you. Please let Anthony know about new Litmus tests once they have been created so that new tests can be created.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Use old toolkit UI (v1) [checked-in]

Anthony, if the patch passes review please make sure to land it on default before the merge. That way we can make sure that those tests can be merged to aurora.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 617839 [details] [diff] [review]
Use old toolkit UI (v1) [checked-in]

Its a + 
Enables the tests and a bit of optimizing here and there. 
Thanks for the patch Henrik!

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 617839 [details] [diff] [review]
Use old toolkit UI (v1) [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/314beae3a54b

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Backport (aurora) v1 [checked-in]

Vlad, if you are around earlier feel free to give me feedback. I want to land this ASAP so our todays tests will not be affected.

Comment 24

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 618172 [details] [diff] [review]
Backport (aurora) v1 [checked-in]

Looks good.
On a side note, for further reviews, please use r=alex.lakatos in the commit messages to be consistent with other reviews addressed to me.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 618172 [details] [diff] [review]
Backport (aurora) v1 [checked-in]

Landed on aurora as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e551b5bfa102