631246 - Save File button on Download Unknown Filetype dialog fails in testDownloadStates.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[u279076]

Error: "Save File radio button on the Download Unknown Type dialog has been selected"
File: testDownloadManager/testDownloadStates.js:64

Comment 1

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

So this is caused of a minimalism dialog. Something I have already mentioned on the other bug. Could be a timing issue in handling the window. You can put a loop around the test function and execute it about 100 times. I assume it will make it visible.

Comment 2

[u279076]

Attachment: Screenshot: empty dialog

I can confirm this error only occurs when the empty Save File dialog appears (see screenshot).  Out of 100 test runs, the empty dialog at least 20 times.  However, most of the time, Mozmill still acts on that dialog successfully.  This makes me think the elements are there, they just aren't rendered on screen.

This makes me think of a couple different possible scenarios:
1) It's an issue with running in a VM
2) It's a real but rare Firefox bug
3) It's a real but rare Mozmill bug
4) It's a combination of the three

The only way we can eliminate #1 is to get someone with Ubuntu 10.10 64-bit to run the same test I've done here.  The other 3 will be harder to eliminate.

Henrik, please advise.

Comment 3

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

Have you tried it on your Mac host? I'm sure that I already have seen it in the past outside of a VM. Can you add a sleep where it is possible, just to check if we aren't too fast.

Comment 4

[u279076]

In addition, this makes me think it would be really valuable to have Mozmill take a screenshot at the instance any failure occurs and report that to Brasstacks.  It would make it a lot easier to catch and debug these issues.

Comment 5

[u279076]

(In reply to comment #3)
> Have you tried it on your Mac host? I'm sure that I already have seen it in the
> past outside of a VM. Can you add a sleep where it is possible, just to check
> if we aren't too fast.

FWIW, I've only ever seen this empty dialog on Linux.

Comment 6

[u279076]

(In reply to comment #5)
> (In reply to comment #3)
> > Have you tried it on your Mac host? I'm sure that I already have seen it in the
> > past outside of a VM. Can you add a sleep where it is possible, just to check
> > if we aren't too fast.
> 
> FWIW, I've only ever seen this empty dialog on Linux.

Scratch that, it is more rare on Mac, (took over 40 testruns before I saw it) but it does happen.  I think we should refactor DownloadManager.downloadFileOfUnknownType() to verify some window content.  It's not simply enough for us to check of an instance of the window.

Comment 7

[u279076]

This seems to be a Firefox issue which Shawn is trying to investigate (see bug 631304).  I think we should probably disable this test until that bug is resolved.

Henrik, please advise.

Comment 8

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

Yes, go for it.

Comment 9

[u279076]

Attachment: Patch v1 (disable-test)

Patch to disable testDownloadStates.js until bug 631034 is fixed.

Comment 10

[u279076]

Comment on attachment 510645 [details] [diff] [review]
Patch v1 (disable-test)

Henrik, can you please spotcheck?  This is the first time I've disabled a test.

Comment 11

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

Now that bug 631304 seems to really trivial to fix, I would propose we go directly with the module fix instead of disabling/enabling tests. Anthony, would you have time for it today?

Comment 12

[u279076]

(In reply to comment #11)
> Now that bug 631304 seems to really trivial to fix, I would propose we go
> directly with the module fix instead of disabling/enabling tests. Anthony,
> would you have time for it today?

Well, the patch for bug 631304 will still result in a failure in this test.  We'll just fail with something like "Full Save File UI is loaded".  We should still disable this test until we can reliably get the Full UI.

Comment 13

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

Why? We only have to select the save radio button if the normal box is displayed. Otherwise we directly click on save. Simply add an if condition.

Comment 14

[u279076]

(In reply to comment #13)
> Why? We only have to select the save radio button if the normal box is
> displayed. Otherwise we directly click on save. Simply add an if condition.

Oh, that's a good point. Ok, cancel my review on the patch.  I'll work out the handling of this dialog in DownloadsAPI first then we can refactor this test.

Comment 15

[u279076]

(In reply to comment #14)
> (In reply to comment #13)
> > Why? We only have to select the save radio button if the normal box is
> > displayed. Otherwise we directly click on save. Simply add an if condition.
> 
> Oh, that's a good point. Ok, cancel my review on the patch.  I'll work out the
> handling of this dialog in DownloadsAPI first then we can refactor this test.

Should I even need to add the check for dialog type to the API? Maybe it's enough to simply do it in this test?  What do you think, Henrik?

Comment 16

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

Well handling this type of window is already part of the shared module. So I believe the question has been answered itself. :)

Comment 17

[u279076]

(In reply to comment #16)
> Well handling this type of window is already part of the shared module. So I
> believe the question has been answered itself. :)

What?...

Comment 18

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

http://hg.mozilla.org/qa/mozmill-tests/file/default/shared-modules/downloads.js#l355

Comment 19

[u279076]

(In reply to comment #18)
> http://hg.mozilla.org/qa/mozmill-tests/file/default/shared-modules/downloads.js#l355

Ok, so I know we need to somehow report shouldntRememberChoice and openWithDefaultOK() within this method.  How do you want this done?

Comment 20

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

You simply check for the normalBox not collapsed. According to the Mochitest you can access it via ID.

Comment 21

[u279076]

I added the following code to http://hg.mozilla.org/qa/mozmill-tests/file/default/shared-modules/downloads.js#l363 (first line within utils.handleWindow call):

var basicBox = new elementslib.ID(controller.window.document, "basicBox");
var normalBox = new elementslib.ID(controller.window.document, "normalBox");
if (basicBox.getNode().collapsed && !normalBox.getNode().collapsed)
  dump("\n *** isNormalBox *** \n");
else
  dump("\n *** isBasicBox *** \n");

The result is that even when we get the Simple UI, it still reports isNormalBox.  This tells me that sometimes we are getting the Simple UI even with basicBox.collapse is TRUE and normalBox.collapsed is FALSE.

Comment 22

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

Can you dump both values at the same time? Also in cases where basicBox is false the above condition will always be false and jump to the else case. It should be enough to check for normalBox not collapsed.

Comment 23

[u279076]

var normalBox = new elementslib.ID(controller.window.document, "normalBox");
if (!normalBox.getNode().collapsed)
  dump("\n *** isNormalBox *** \n");
else
  dump("\n *** isBasicBox *** \n");

It still reported the Simplified UI as a normalBox.  I'll try to come up with a minimized testcase so you can see.

Comment 24

[u279076]

Attachment: testDownloadStates-simplified.js

Here is a simplified version of testDownloadStates.  Henrik, please test locally and you will see what I mean.

You should notice that under all conditions (whether we see the Normal UI or the Simple UI), normalBox.collapsed is always FALSE and basicBox.collapsed is always TRUE.

Comment 25

[u279076]

(In reply to comment #24)
> Created attachment 510732 [details]
> testDownloadStates-simplified.js
> 
> Here is a simplified version of testDownloadStates.  Henrik, please test
> locally and you will see what I mean.
> 
> You should notice that under all conditions (whether we see the Normal UI or
> the Simple UI), normalBox.collapsed is always FALSE and basicBox.collapsed is
> always TRUE.

Sorry, I should move this over to bug 631304.

Comment 26

[u279076]

Attachment: Patch v1 [checked-in]

This patch adds checking for the Normal vs Simplified version of the Save File dialog.  If we get a normal version we have an additional step to select the Save File radio.

Comment 27

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

Comment on attachment 510742 [details] [diff] [review]
Patch v1 [checked-in]

># HG changeset patch
># User Anthony Hughes <[e-mail]>
># Date 1297198806 28800
># Node ID a76b50fb8dc44fa6b793bb1f8adac9cf939c10ce
># Parent 945880f4a2a4179b028d8a636ec35ce281df5b75
>Bug 631246 - add handling of simplified/normal Save File dialog to DownloadAPI. r=hskupin
>
>diff --git a/shared-modules/downloads.js b/shared-modules/downloads.js
>--- a/shared-modules/downloads.js
>+++ b/shared-modules/downloads.js
>@@ -355,22 +355,27 @@ downloadManager.prototype = {
> var downloadFileOfUnknownType = function(controller, url) {
>   controller.open(url);
> 
>   // Wait until the unknown content type dialog has been opened
>   controller.waitForEval("subject.getMostRecentWindow('').document.documentElement.id == 'unknownContentType'",
>                          gTimeout, 100, mozmill.wm);
> 
>   utils.handleWindow("type", "", function (controller) {
>-    // Select to save the file directly
>-    var saveFile = new elementslib.ID(controller.window.document, "save");
>-    controller.waitThenClick(saveFile, gTimeout);
>-    controller.waitFor(function () {
>-      return saveFile.getNode().selected;
>-    }, "Save File radio button on the Download Unknown Type dialog has been selected");
>+    var normalBox = new elementslib.ID(controller.window.document, "normalBox");
>+    
>+    // Check if we have a normal dialog or simplified dialog
>+    if (!normalBox.getNode().collapsed) {
>+      // We have a normal dialog so click the Save File radio first
>+      var saveFile = new elementslib.ID(controller.window.document, "save");
>+      controller.waitThenClick(saveFile, gTimeout);
>+      controller.waitFor(function () {
>+        return saveFile.getNode().selected;
>+      }, "Save File radio button on the Download Unknown Type dialog has been selected"); 
>+    }
> 
>     // Wait until the OK button has been enabled and click on it
>     var button = new elementslib.Lookup(controller.window.document,
>                                         '/id("unknownContentType")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');
>     controller.waitForElement(button, gTimeout);
>     controller.waitForEval("subject.okButton.hasAttribute('disabled') == false", gTimeout, 100,
>                            {okButton: button.getNode()});
>     controller.click(button);

Comment 28

[u279076]

(In reply to comment #27)
> Comment on attachment 510742 [details] [diff] [review]
> Patch v1

Was there a comment you meant to leave, or is this patch ok to land as-is?

Comment 29

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

No comment == everything fine. Just get it landed, and we will see how it works. If failures still appear we will have to disable the test until the problem on bug 631304 has been identified and fixed.

Comment 30

[u279076]

Comment on attachment 510742 [details] [diff] [review]
Patch v1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e282e5c59b9c [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/88f208c287e9 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/02cb321d92e7 [mozilla1.9.1]

Comment 31

[u279076]

Keeping this open for now.  If we see no failures in the next week we can mark resolved.  Otherwise, we'll need to disable the test.

Comment 32

[u279076]

As I suspected, the failure still exists (Feb 11).  I had a feeling this wouldn't solve anything because we visually see the simple UI but Firefox/Mozmill thinks it's the normal UI.  We'll have to figure out why on bug 631304.

I'll go ahead with disabling this test in the meantime.

Comment 33

[u279076]

Attachment: Patch v2 (disable) [checked-in]

Patch to disable testDownloadStates until bug 631304 is fixed.

Comment 34

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

Comment on attachment 512895 [details] [diff] [review]
Patch v2 (disable) [checked-in]

Please also remove the Mozmill group assignment for all the branches on Litmus.

Comment 35

[u279076]

Comment on attachment 512895 [details] [diff] [review]
Patch v2 (disable) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/db8880351f5b [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/c7ee221a5ce5 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/89d2249107a0 [mozilla1.9.1]

Comment 36

[u279076]

Found and updated the following tests in Litmus:
 * https://litmus.mozilla.org/show_test.cgi?id=10466
 * https://litmus.mozilla.org/show_test.cgi?id=10451
 * https://litmus.mozilla.org/show_test.cgi?id=8463
 * https://litmus.mozilla.org/show_test.cgi?id=6426

Comment 37

[u279076]

Attachment: Patch v2.1 (disable) [checked-in]

Forgot to disable the teardown module -- here is the follow up patch.

Comment 38

[u279076]

Comment on attachment 513516 [details] [diff] [review]
Patch v2.1 (disable) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/36e010271ba6 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/18dd064a7946 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/0ff02d57354f [mozilla1.9.1]

Comment 39

[u279076]

Attachment: Patch v3

Patch to re-enable test and add handling of the Save File radio button only in cases where it exists.  This will most likely be a wait-and-see fix.

Comment 40

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

Comment on attachment 514329 [details] [diff] [review]
Patch v3

>       controller.waitFor(function () {
>-        return saveFile.getNode().selected;
>+        controller.click(saveFileRadio);
>+        return saveFileRadio.getNode().selected;
>       }, "Save File radio button on the Download Unknown Type dialog has been selected"); 

We only want to click once on the radio button. Simply move it right before the waitFor.

Further you should run a test over 100 cycles to ensure it works. It was easy for me to get that failure on OS X with a latest trunk nightly, so proofing that it works before we check-in the fix would be a great addition.

Comment 41

[u279076]

Attachment: Patch v3.1 [checked-in]

Moved the click outside of waitFor().  I also ran this through a for loop of 1000 iterations and it didn't fail.  Looks to be a fairly safe workaround until we can get bug 631304 sorted out.

Comment 42

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

Comment on attachment 514415 [details] [diff] [review]
Patch v3.1 [checked-in]

Sounds great and I also think it should be save enough for the time being! Thanks Anthony

Comment 43

[u279076]

Comment on attachment 514415 [details] [diff] [review]
Patch v3.1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/648a2cdb1d8a [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/d39fb7ce29f6 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/f3c99d83fe82 [mozilla1.9.1]

Comment 44

[u279076]

Marking resolved fixed.  Will mark verified if we don't see this failure after a week.

Comment 45

[u279076]

http://mozmill-release.brasstacks.mozilla.com/#/general/report/646aeed771f23fa7fe587d977d4b6758

Well, it only took a day, but it failed on Ubuntu 10.10 32-bit (same VM as the testPasswordNotSaved.js intermittent orange - bug 614973).  I'd like to wait for a few more days of results before we call this one intermittent orange as well.  However, I suspect this might be a VM issue since I tested this 1000 times without fail in my local VM.

Comment 46

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

Please get this disabled for now on all platforms. We also failed on Windows today:

http://mozmill-release.brasstacks.mozilla.com/#/general/report/aa753950c09ba4cde29deade2774f335

Comment 47

[u279076]

Attachment: Patch v4 (disable)

Patch to disable testDownloadStates

Comment 48

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

Comment on attachment 516909 [details] [diff] [review]
Patch v4 (disable)

>+ // Bug 631246: Test randomly displays a simplified version of the Save File dialog
>+ //             but programmatically expects the full version of the Save File dialog
>+ setupModule.__force_skip__ = "Bug 631246: Unexpected SIMPLE version of Save File dialog";
>+ teardownModule.__force_skip__ = true;
>\ No newline at end of file

r=me with a new line added at the end.

Comment 49

[u279076]

Attachment: Patch v4.1 (disable)

Comment 50

[u279076]

Attachment: Patch v4.1 (disable) [checked-in]

Comment 51

[u279076]

Comment on attachment 516925 [details] [diff] [review]
Patch v4.1 (disable) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/230309f16f34 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/5d5aa39f878d [mozilla2.0]
http://hg.mozilla.org/qa/mozmill-tests/rev/1612d427fbb2 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/29dcdb7f7889 [mozilla1.9.1]

Comment 52

[u279076]

Should we try re-enabling this test now the the VM issue "appears" to have been resolved?

Comment 53

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

No, it's not related to a slow Ubuntu box. So we can't enable the test until the underlying issue has been fixed.

Comment 54

[u279076]

I created a 10MB test file with a fictional extension:
http://people.mozilla.com/~ahughes/mozmill/test-files/

We discussed last week that the Private Browsing download test does not seem to suffer from this failure. Since we are using a tar.gz file currently, this could be treated as "known" under certain conditions (we always want "unknown").

For reference, the following is the command I used to generate the file:
dd if=/dev/zero of=unknown_filetype_10MB.mfbt bs=1024 count=0 seek=$[1024*10]

Henrik, do you think this file is sufficient for us to try in the download tests?

Comment 55

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

No, we want to have a Python script which creates this random data and sends it as a file to the client. But please use your file to test if that fixes the issue we are experiencing at the moment on the affected boxes. Could be helpful to even fix the core issue.

Comment 56

[u279076]

(In reply to comment #55)
> No, we want to have a Python script which creates this random data and sends it
> as a file to the client. But please use your file to test if that fixes the
> issue we are experiencing at the moment on the affected boxes. Could be helpful
> to even fix the core issue.

I'm not sure I understand your rationale behind that. Would we be calling the python script from within the Mozmill test? And how to we perform the act of clicking the link to a file "sent to the client"? Please elaborate, thanks.

Also, I don't think I can test this without re-enabling the test and waiting to see if it fails in the daily testrun. I was unable to ever reproduce the failure locally, even when wrapping in a 1000 iteration for loop.

Comment 57

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

The script would be available as i.e. http://www.mozqa.com/data/firefox/general/randomdata.php and will serve a file like test.mbgr which contains random data. We can even give an argument like '?size=512000' to send a file which is 500KB in size.

I was able to reproduce this issue even in the 3rd or 4th iteration on OS X. So I wonder why you aren't able to. Create a patch and I can try to run it when I have time later this week.

Comment 58

[u279076]

Attachment: Minimized testcase

Here is a patch for a minimized test case which iterates 100 times trying to download my new test data file.

FWIW, when I said "I'm not able to reproduce", I did not mean that I'm unable to see the simplified UI. I see the normal UI sometimes and the simplified UI sometimes.  This was never the problem.  The problem was that on rare occasions, the simplified UI was causing a failure (not all the time).  This failure is what I can never reproduce.

Comment 59

[Raul Nicolae Malea]

http://mozmill-crowd.brasstacks.mozilla.com/#/general/report/b0c5cbbd24bf827d6dcfffc745f80c73

Comment 60

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

Comment on attachment 521572 [details] [diff] [review]
Minimized testcase

I don't have the time for deep testing at the moment. So can you please take the former URL and check which VM on horus and which version of Firefox always show this failure? That would be necessary to test this minimized testcase.

Comment 61

[u279076]

According to http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=All&platform=All&from=2011-03-01&to=2011-03-29&test=%2FtestDownloading%2FtestDownloadStates.js&func=testDownloadStates this used to happen most frequently on Ubuntu 10.10 32-bit on Minefield.

I'll try to reproduce it there.

Comment 62

[u279076]

This failure has no recent reports -- resolving WORKSFORME.

Comment 63

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #62)
> This failure has no recent reports -- resolving WORKSFORME.

Right, because it has been marked as skipped. There are two options: Leave this bug open until the issue is fixed or file a new bug to get the test un-skipped again. The former option is the way how we should handle it.

Comment 64

[u279076]

I'd like to see if this test is failing on the new branches (default, aurora, beta, and release). This test was skipped before we started the rapid-release process. If it can only be reproduce on the older branches I would say skip on those branches and mark this bug as WONTFIX.

What do you think Henrik?

Comment 65

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

I don't think that something has been changed in the last months. But feel free to test locally.

Comment 66

[u279076]

I'm unable to reproduce this locally. I propose we try re-enabling this test with refactored code to use the new assertions module on default and see how it does on a live testrun. If it passes, we renable it on other branches. If it fails, at least we get some new data and can reopen investigation using the latest Nightly.

Comment 67

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

So try it out and lets cross fingers that we would get more information. If it passes we should let it run a couple of days just to be sure about the results.

Comment 68

[Daniel Gherasim]

Not sure if the problem with empty "Save File Dialog" still reproduces, but we will handle downloads in bug 908649 & after that we can get this test updated & unskip if all is ok.

Comment 69

[Teodor Druta]

This was fixed by Bug 908649.