614973 - Expression is NULL failure in testPasswordNotSaved

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

MODULE: testPasswordManager/testPasswordNotSaved.js
TEST: testPasswordNotificationBar
ERROR: Expression "{"value":"password-save"}" returned null. Anonymous == false
BRANCH: mozilla1.9.1
PLATFORM: Linux 10.04

Comment 1

[u279076]

Attachment: Patch v1 (mozilla1.9.1)

Seems like the failure is actually in testPopupBlocked.  We are incorrectly looking for password-save instead of popup-blocked notification bar.  Here is a patch which fixes this.

Comment 2

[Aaron Train [:aaronmt]]

Comment on attachment 493418 [details] [diff] [review]
Patch v1 (mozilla1.9.1)

Strange. Thanks.

Comment 3

[u279076]

(In reply to comment #1)
> Created attachment 493418 [details] [diff] [review]
> Patch v1 (mozilla1.9.1)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/5d042b4155b4 [mozilla1.9.1]

Comment 4

[u279076]

Looks like this is still happening and is being reported on mozilla1.9.2 and mozilla1.9.1 now.  The fix I applied in comment 3 was needed but was not the cause of the issue, apparently.

Reopening to investigate what is going on with the password notification bar.

Comment 5

[u279076]

Again, this is linux-only.  I've attached a URL to the brasstacks report showing the failures.

Comment 6

[u279076]

After several attempts, I am unable to reproduce this locally.  I'll try investigating using qa-set.

Comment 7

[u279076]

I'm unable to reproduce this on qa-set either.  Henrik, is it possible this is a problem with QA-Horus? Perhaps it needs to be restarted? I seem to remember seeing odd issues before in daily-testruns where restarting the VMs and the box fixed it.

Comment 8

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

Hm, this Linux VM was the only VM which was made visible on this machine, and there was a shell window. I have closed the window and minimized the VM. Lets see if this has changed anything in todays testrun.

Comment 9

[u279076]

Still failing on that VM and nowhere else.  Seems it is failing on the Linux32 VM only, not the Linux64 VM.  In fact, I can't find this failure at all going back 10 days on the Linux64 VM -- only the Linux32 VM.  This makes me think something is wrong with the VM itself.  Any chance we can restart the VM and see if it self-corrects itself?

Comment 10

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

Go ahead. Simply check tomorrows results to proof.

Comment 11

[u279076]

(In reply to comment #10)
> Go ahead. Simply check tomorrows results to proof.

Restarting the VM seems to have cleared the failure on 3.6 but it is still occurring on 3.5. I'll have to investigate this locally on qa-horus.

Comment 12

[u279076]

Two things I've noticed while investigating the VMs:

Ubuntu64 - I'm seeing a JSBridge component failure when launching Mozmill:
jsbridge: Exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIServerSocket.init]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///tmp/tmp8b_mWm.mozrunner/extensions/[e-mail]/resource/modules/server.js :: anonymous :: line 279"  data: no]

Ubuntu32 - I'm seeing a setuptools error when launching Mozmill:
/usr/local/bin/mozmill:5: UserWarning: Unbuilt egg for setuptools [unknown version] (/usr/lib/python2.6/dist-packages)
  from pkg_resources import load_entry_point

I have no idea if these impact the reliability of the tests, but I don't see these errors on the qa-set VMs or my local VMs.

Comment 13

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

(In reply to comment #12)
> Ubuntu64 - I'm seeing a JSBridge component failure when launching Mozmill:
> jsbridge: Exception: [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIServerSocket.init]"  nsresult: "0x80004005

there is a running instance. Simply kill all jsbridge processes or the browser, if it is still open.

> Ubuntu32 - I'm seeing a setuptools error when launching Mozmill:
> /usr/local/bin/mozmill:5: UserWarning: Unbuilt egg for setuptools [unknown
> version] (/usr/lib/python2.6/dist-packages)
>   from pkg_resources import load_entry_point
> 
> I have no idea if these impact the reliability of the tests, but I don't see
> these errors on the qa-set VMs or my local VMs.

I will look at this.

Comment 14

[u279076]

> Ubuntu32 - I'm seeing a setuptools error when launching Mozmill:
> /usr/local/bin/mozmill:5: UserWarning: Unbuilt egg for setuptools [unknown
> version] (/usr/lib/python2.6/dist-packages)
>   from pkg_resources import load_entry_point
> 
> I have no idea if these impact the reliability of the tests, but I don't see
> these errors on the qa-set VMs or my local VMs.

> I will look at this.

I fixed this error...running a daily testrun now to see if it changes our brasstacks results.

Comment 15

[u279076]

Well, that didn't fix this failure.  Continuing to investigate...

Comment 16

[u279076]

Ok, I have something interesting.  

If I execute a manual testrun, the test passes:
mozmill -b <build> -t ./firefox/

If I execute a daily testrun, the test fails:
testrun_general.py <build>

Any ideas, Henrik?

Comment 17

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

I recall that the order of tests is somewhat mixed-up on Linux. Can you please check if that happens here?

Comment 18

[u279076]

(In reply to comment #17)
> I recall that the order of tests is somewhat mixed-up on Linux. Can you please
> check if that happens here?

testNavigateFTP happens before testPasswordNotSaved, same as on other platforms.

Comment 19

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

(In reply to comment #18)
> testNavigateFTP happens before testPasswordNotSaved, same as on other
> platforms.

So what you can do is, use your cloned version of the automation scripts and open lib/testrun.py. Search for:

self.test_path = os.path.join(self._base_path, 'tests')

And replace it with 

self.test_path = os.path.join(self._base_path, 'tests', 'testPasswordManager')

That will only execute the specified folder then. Does that change anything?

Comment 20

[u279076]

(In reply to comment #19)
> (In reply to comment #18)
> > testNavigateFTP happens before testPasswordNotSaved, same as on other
> > platforms.
> 
> So what you can do is, use your cloned version of the automation scripts and
> open lib/testrun.py. Search for:
> 
> self.test_path = os.path.join(self._base_path, 'tests')
> 
> And replace it with 
> 
> self.test_path = os.path.join(self._base_path, 'tests', 'testPasswordManager')
> 
> That will only execute the specified folder then. Does that change anything?

I did this but it still ran all of the tests, resulting in the same failure.

Comment 21

[u279076]

Actually, I got it to run just the Password Manager tests by changing the following:

http://hg.mozilla.org/qa/mozmill-automation/file/1f4778986d18/libs/testrun.py#l358
self.test_path = os.path.join('firefox/testPasswordManager')

The result is all 8 of the Password Manager tests pass.  Henrik, please advise.

Thanks.

Comment 22

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

Can you please attach the full list of tests which get run in-front of this failing one, and that for both ways of execution? Looks like a test from another subfolder could be causing this.

You can also pull the hotfix-1.5.2, where you now can specify multiple tests/folders with --tests. That should make it easier to build-up a list of tests to run in the specified order.

Comment 23

[u279076]

It definitely seems to be some sort of ordering condition which is unique to running the testrun_general.py script on the Linux32 VM...

Under all testing conditions, except for testrun_general.py on Linux32, the order of execution is the order on the filesystem (alphabetical).  However, when you run testrun_general.py on the Linux32 VM, the order is all over the map:

testAddons
testAwesomeBar
testBookmarks
testCookies
testDownloading
testFindInPage
testFormManager
testGeneral
testInstallation
testLayout
testPasswordManager
testPopups
testPreferences
testPrivateBrowsing
testSearch
testSecurity
testSessionStore
testTabbedBrowsing
testTechnicalTools
testToolbar

- BECOMES - 

testSessionStore
testBookmarks
testCookies
testTechnicalTools
testInstallation
testPreferences
testFormManager
testLayout
testPasswordManager
testAwesomeBar
testSecurity
testFindInPage
testPopups
testSearch
testTabbedBrowsing
testToolbar
testAddons
testGeneral
testDownloading
testPrivateBrowsing

Comment 24

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

(In reply to comment #23)
> Under all testing conditions, except for testrun_general.py on Linux32, the
> order of execution is the order on the filesystem (alphabetical).  However,
> when you run testrun_general.py on the Linux32 VM, the order is all over the
> map:

That's strange, because I do only forward the folder name to Mozmill. There is not happening any mixing of folders on the automation script side. To investigate this please file a new bug for it under automation.

To get this bug solved, please checkout the latest version of Mozmill from the hotfix-1.5.2 branch. By using it you can specify multiple -t options. That way you can force Mozmill to exactly run the same order of tests as the general test-run does. It should help to nail down this problem.

Comment 25

[u279076]

I think I'm not doing this right because it is only running the last folder.

Here is my command:
(mozmill1.5.2-venv)host-3-58:firefox ashughes$ mozmill -b /Applications/Shiretoko.app/ -t testSessionStore -t testBookmarks -t testCookies -t testTechnicalTools -t testInstallation -t testPreferences -t testFormManager -t testLayout -t testPasswordManager --show-errors

I'm using mozmill-1.5.2pre from:
k0s-mozmill-e0f995a

The only tests that execute are the ones in testPasswordManager, nothing else is even executed.

Comment 26

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

(In reply to comment #25)
> I'm using mozmill-1.5.2pre from:
> k0s-mozmill-e0f995a

You shouldn't use Jeff's repository. Get the latest revision from :
https://github.com/mozautomation/mozmill/tree/hotfix-1.5.2

Comment 27

[u279076]

(In reply to comment #26)
> (In reply to comment #25)
> > I'm using mozmill-1.5.2pre from:
> > k0s-mozmill-e0f995a
> 
> You shouldn't use Jeff's repository. Get the latest revision from :
> https://github.com/mozautomation/mozmill/tree/hotfix-1.5.2

Moving this issue over to bug 623992.

Comment 28

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

Just to note, even with the mixed order our tests should not fail. Means we should find the root cause of this problem and get the right test fixed.

Comment 29

[u279076]

So, I've run the tests using hotfix-1.5.2, and there are no failures whatsoever.  Henrik, can you get the testrun_general.py script working so I can try it with hotfix-1.5.2?

Right now, the script only starts Firefox, no tests run.

Comment 30

[u279076]

(In reply to comment #29)
> So, I've run the tests using hotfix-1.5.2, and there are no failures
> whatsoever.  Henrik, can you get the testrun_general.py script working so I can
> try it with hotfix-1.5.2?
> 
> Right now, the script only starts Firefox, no tests run.

Sorry, this was meant for bug 623992.

Comment 31

[u279076]

Using hotfix and libs/testrun.py set to only run testPasswordManager tests, the test fails via script, but passes if run with Mozmill.

mozmill -b <build> -t firefox/testPasswordManager: PASS

testrun_general.py <build>: FAIL

Henrik, do you have any advice on how I can troubleshoot this further?

Comment 32

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

(In reply to comment #31)
> Using hotfix and libs/testrun.py set to only run testPasswordManager tests, the
> test fails via script, but passes if run with Mozmill.

How does your change look like?

Comment 33

[u279076]

http://hg.mozilla.org/qa/mozmill-automation/file/1f4778986d18/libs/testrun.py#l358
self.test_path = os.path.join('firefox/testPasswordManager')

Comment 34

[u279076]

(In reply to comment #33)
> http://hg.mozilla.org/qa/mozmill-automation/file/1f4778986d18/libs/testrun.py#l358
> self.test_path = os.path.join('firefox/testPasswordManager')

Sorry, that should be
http://hg.mozilla.org/qa/mozmill-automation/file/tip/libs/testrun.py#l358

Comment 35

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

(In reply to comment #34)
> (In reply to comment #33)
> > http://hg.mozilla.org/qa/mozmill-automation/file/1f4778986d18/libs/testrun.py#l358
> > self.test_path = os.path.join('firefox/testPasswordManager')

You will have to use: os.path.join('firefox', 'testPasswordManager').

Also a full error message would be good. Not sure what you are seeing.

Comment 36

[u279076]

> You will have to use: os.path.join('firefox', 'testPasswordManager').
Done. Error still exists.

> Also a full error message would be good. Not sure what you are seeing.
Test Failure: {"exception": {"message": "Unexpectedly found element Lookup: /id(\"main-window\")/id(\"mainPopupSet\")/id(\"notification-popup\")/id(\"password-save-notification\")"

Comment 37

[u279076]

I'm starting to think this is something unique to either qa-horus or the Ubuntu VM.  

On qa-horus, the error is:
Expression "{"value":"password-save"}" returned null. Anonymous == false

I cannot reproduce the error on my Ubuntu 10.10 VM.  I'm currently copying over the 10.04 VM (same as the one qa-horus uses).

Comment 38

[u279076]

This seems to only fail on qa-horus.  I've tried to reproduce this failure locally on Ubuntu 10.10, Ubuntu 10.04, on qa-set, and on qa-horus.  The only place it is reproducible on qa-horus.

Henrik, can you think of any reason why the test would only fail on qa-horus?

Comment 39

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

Not out of my head, but you should probably dump as many as possible data to the console right before the failing line. If you can prepare the test-run so I could start it with a simple command, I could offer my help in investigation.

Comment 40

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

Any update here?

Comment 41

[u279076]

(In reply to comment #40)
> Any update here?

Nothing new yet.  I've been focusing on Firefox 4.0b9 testing this week.  It should be off my plate by noon PDT today.  This bug is my top Mozmill priority.

Comment 42

[u279076]

Attachment: Patch v1 (default)

Refactor test to use waitFor() instead of assert().

I've created this patch to refactor the assertion after the ESC key is pressed to close the notification bar.  The error we are receiving indicates that notificationBar.isNull in the assert().  This makes me think the assert() is not fired fast enough (ie. the assert is being fired after the notification bar has already been destroyed).  Hopefully a waitFor() will be more robust here.

Comment 43

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

Comment on attachment 504044 [details] [diff] [review]
Patch v1 (default)

>   controller.keypress(passwordNotification, "VK_ESCAPE", {});
>-  controller.assert(function() {
>-    return passwordNotification.getNode().parentNode.state == "closed";
>-  }, "Password notification should be closed");
>+  controller.waitFor(function() {

A waitFor makes really sense here because we have a slight animation. There has to be a space between function and the opening bracket.

>+    return passwordNotification.getNode().parentNode.state === "closed";
>+  }, "PasswordNotificationBar.State: got '" +
>+     passwordNotification.getNode().parentNode.state + "' expected 'closed'");

The message should be formal text followed by " - got XXX, expected: YYY", i.e. "Password notification bar has been closed...".  

r=me with both updated.

Comment 44

[u279076]

Attachment: Patch v1.1 (default)

Patch with revisions applied.

Comment 45

[u279076]

(In reply to comment #44)
> Created attachment 504068 [details] [diff] [review]
> Patch v1.1 (default)
> 
> Patch with revisions applied.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/029aa329b8be (default)

Comment 46

[u279076]

I can't directly backport this patch yet because I've found a rather large discrepency between the test on default vs branch.  I'll attach the two files so they can be diff'd.

Comment 47

[u279076]

Attachment: testPasswordNotSaved.js (default)

Comment 48

[u279076]

Attachment: testPasswordNotSaved.js (mozilla1.9.2)

Comment 49

[u279076]

Attachment: Screenshot: diff

That didn't work exactly how I liked it, so here is a screenshot from Komodo showing the files side by side; default is on the left, mozilla1.9.2 is on the right.

Comment 50

[u279076]

Nevermind all that, I forgot that DEFAULT uses a doorhanger, whereas BRANCH uses a notification bar.

I'll create a separate patch for branch.

Comment 51

[u279076]

(In reply to comment #50)
> Nevermind all that, I forgot that DEFAULT uses a doorhanger, whereas BRANCH
> uses a notification bar.
> 
> I'll create a separate patch for branch.

On a side note, I think it might be worth refactoring the DEFAULT version of this test to remove references to "notification bar" as this is what caused my confusion initially.

Comment 52

[Aaron Train [:aaronmt]]

Agreed, anyone reading these tests would undoubtedly be confused by the false comments without a refactor.

Comment 53

[u279076]

waitForElementNotPresent (bug 610757) does not fix this error on qa-horus. I propose the following:

1. Refactor test on default to remove references to "bar"
2. Refactor test on branch to yield more valuable error messages
3a. If that gives us more clues, investigate and fix
3b. If not, disable the test until we can find a fix

Henrik, what do you think?

Comment 54

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

Sure, but we should probably start with the branch tests, because those are failing while default doesn't show the failure. If you can come up with a patch I will make sure to land it if it gets r+ before tomorrows testrun.

Comment 55

[u279076]

I vote we simply just disable this test RE: https://bugzilla.mozilla.org/show_bug.cgi?id=623992#c16

Going back to the list in comment 53, 

1. Refactor test on default to remove references to "bar"
 - we can still do this easily
2. Refactor test on branch to yield more valuable error messages
 - there really isn't anything to refactor here.  We have waitFor() in place via getTabPanelElement().  The error which is reporting to brasstacks is actually coming from elementslib.

I reiterate, we should remove "bar" from default and disable this test on branch until bug 623992 is resolved.

Henrik, what do you think?

Comment 56

[u279076]

One other thing I noticed during debugging, testAccessPageInfo.js does not cleanly destruct.  It leaves the Page Info dialog hanging around.  This doesn't seem to have any bearing on the testrun, but we should make sure it cleanly destructs anyway.

Henrik, should I file a bug for this?

Comment 57

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

The issue here is exactly the testAccessPageInfoDialog.js test. Once it does not open the window, everything is fine. See my tests:

With page info window opened:
http://mozmill-crowd.brasstacks.mozilla.com/#/general/report/90365723b3905a8fdc8895b9b805a6b2

Page info window not opened:
http://mozmill-crowd.brasstacks.mozilla.com/#/general/report/90365723b3905a8fdc8895b9b805ab9d

That means fixing testAccessPageInfoDialog.js will also fix this bug. Can you create a patch Anthony? And thanks for the investigation.

Comment 58

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

As what I have also seen on the Linux VM on horus, we have problems with HTTPd. In some cases we cannot connect. That gives me the impression that not only the page info window is responsible for the failure. I have removed it and the failure persists:

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

So lets get the page info clean-up work happen on its own bug, so we can check-in the patch ASAP. Then we can check what has been changed for this test failure.

Comment 59

[u279076]

testAccessPageInfoDialog.js: bug 626674

Comment 60

[u279076]

Using the refactoring done in bug 626674, we now fail with a waitFor() directly in the test (no longer in elementslib with an obscure exception).  However, we do get a strange scenario where the GOT is the same as EXPECTED but still fail.

Here is the code:
// The notification bar should not be visible before submitting the credentials
  var button = null;
  controller.waitFor(function () {
    button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex, '/{"value":"password-save"}/anon({"type":"info"})/{"class":"messageCloseButton tabbable"}');  
    return button === null;
  }, "Checking for notification bar close button: got " + button + ", expected null");

Here is the error message:
Checking for notification bar close button: got null, expected null

Comment 61

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

Shouldn't we check here for the notification bar and not the button in general? When you run the test please add a dump statement like the following into the waitFor callback to check the real value of the element:

dump("** button element: " + button + "\n");
dump("** button node: " + button.getNode() + "\n");

That will give us an idea.

Comment 62

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

Also please keep in mind that getTabPanelElement always returns an element, which means that button is always a valid object.

Comment 63

[u279076]

(In reply to comment #61)
> Shouldn't we check here for the notification bar and not the button in general?

This is what I was doing before.  The panel containing notification bars has a .hasNotification property.  It has a value of 0 if none, and 1 or more if one or more.  I don't think it's feasible enough for us to check if > 1 because it will give us a false positive if a notification bar is loaded before password-saved and not closed (ie. Mozilla Rights).  After some discussion, we agreed it would be best to just check for the (X) button of the password-save bar.

Comment 64

[u279076]

Ok, I've added your dumps.

I get 51 instances of the dumps before the waitFor() times out:
 *** button element: [object Object] *** 
 *** button node: [object XULElement]*** 

Again, the error message is:
Checking for notification bar close button: got null, expected null

Comment 65

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

Do you have a patch you can attach, so I can try it tomorrow? Just to make sure we have the same code base. Thanks.

Comment 66

[u279076]

(In reply to comment #65)
> Do you have a patch you can attach, so I can try it tomorrow? Just to make sure
> we have the same code base. Thanks.

A patch to test for this will have to wait for bug 626674. I'll land that asap and then lets look at this bug.

Comment 67

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

This issue is still persistent on Linux on the default branch. I think we should run it with Mozmill 1.5.2 just to see if something will be changed.

Therefore you can find a virtualenv sandbox with the latest hotfix changeset in /data/testing/hskupin/linux-sandbox. Just enable it with "source linux-sandbox/bin/activate". From inside the env you could start the daily test-run, which should report to mozmill-archive.

Comment 68

[u279076]

Attachment: Patch v2

Refactoring assertNodeNotExist into a waitFor seems to have prevented this failure.

Comment 69

[Aaron Train [:aaronmt]]

Comment on attachment 507543 [details] [diff] [review]
Patch v2

Looks fine by me

Comment 70

[u279076]

Comment on attachment 507543 [details] [diff] [review]
Patch v2

Quick spotcheck please, Henrik.

Comment 71

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

Comment on attachment 507543 [details] [diff] [review]
Patch v2

>   // The notification should not be visible before submitting the credentials
>-  controller.assertNodeNotExist(passwordNotification);
>+  controller.waitFor(function () {
>+    return passwordNotification.getNode().parentNode.state === "closed";
>+  }, "Password notification closed: got '" +
>+     passwordNotification.getNode().parentNode.state + "' expected 'closed'");

Why do we need a waitFor here? There shouldn't be any notification open at this time. If that's the case we should really have a reset call in setupModule or teardown.

Comment 72

[u279076]

(In reply to comment #71)
>> Comment on attachment 507543 [details] [diff] [review]
>> Patch v2
>
> Why do we need a waitFor here? There shouldn't be any notification open at this
> time. If that's the case we should really have a reset call in setupModule or
> teardown.

The purpose of this check is to make sure there is no notification before we submit the credentials; it should only appear after (something which you requested be in this test when it was being created).

Comment 73

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

(In reply to comment #72)
> The purpose of this check is to make sure there is no notification before we
> submit the credentials; it should only appear after (something which you
> requested be in this test when it was being created).

Even when a notification would be shown, we wouldn't catch this with a waitFor. The check you are doing immediately returns because it checks for the closed property. The opening of the notification is delayed. So it would slip through. If we want to test this correctly we would have to wait for the notification and catch the thrown exception.

Comment 74

[u279076]

Personally, I never understood why we were checking to make sure a notification does not appear before the Login button is clicked. There is no historical evidence (ie. bug) where this has ever happened.  I'd be perfectly happy to leave this out of the test completely.  

> If we want to test this correctly we would have to wait for the notification
> and catch the thrown exception.

I'm not sure I understand what you are asking for here.  Are you asking to implement a waitFor() which waits for a notification to exists and when it fails to do so we catch that as a PASS, not a FAIL?

Comment 75

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

(In reply to comment #74)
> Personally, I never understood why we were checking to make sure a notification
> does not appear before the Login button is clicked. There is no historical
> evidence (ie. bug) where this has ever happened.  I'd be perfectly happy to
> leave this out of the test completely.  

Well, having no bug doesn't mean it cannot regress by any future change. But such a regression would be highly visible and nightly testers would catch it immediately. Even, as you said, the risk that it will happen is low. So let us remove this part.

> > If we want to test this correctly we would have to wait for the notification
> > and catch the thrown exception.
> 
> I'm not sure I understand what you are asking for here.  Are you asking to
> implement a waitFor() which waits for a notification to exists and when it
> fails to do so we catch that as a PASS, not a FAIL?

For future cases you have understood it correctly. Yes, that's what I have said.

Comment 76

[u279076]

Attachment: Patch v3

With the check for "notification not present before login" removed, we now get a reference to the notification after (not before) the Login button is clicked.

Also, I renamed the test method to testPasswordNotification to remove confusion about bar vs doorhanger.

Comment 77

[Aaron Train [:aaronmt]]

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

>-  // After logging in, close the notification bar
>+  // Click the login button and wait for the form to process
>+  controller.click(new elementslib.ID(controller.tabs.activeTab, "LogIn"));
>+  controller.waitForPageLoad();

Nit: Pull out the element declaration outside the call to click.

Comment 78

[u279076]

Attachment: Patch v4

Comment 79

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

Comment on attachment 507916 [details] [diff] [review]
Patch v4

>   // Close the notification and check its state
>   controller.keypress(passwordNotification, "VK_ESCAPE", {});
>   controller.waitFor(function () {
>     return passwordNotification.getNode().parentNode.state === "closed";
>-  }, "Password notification bar closed: got '" +
>+  }, "Password notification closed: got '" +
>      passwordNotification.getNode().parentNode.state + "' expected 'closed'");

Isn't that a boolean like expression? We can only have two states here, so I don't think we should add the got/expected part. "Password notification has been closed" should be enough IMO.

r+ with it updated.

Comment 80

[u279076]

Ok, I'll make that change.  Before I do though, I have a question about backporting these changes.  A big part of this patch is removing the confusion of "notification bar" vs "doorhanger" by referring to it simply as "notification".  Is there any objections to me backporting that change?

This change would make the tests the same across all branches.

Comment 81

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

I don't see an issue for older branches, but we do not have eliminated all instances of notifications in Firefox 4. That means notifications and doorhangers will co-exist. Sorry, I totally forgot about that part. That said, we have totally different ways to access such content. We could take care of in the shared module refactoring. For now I would agree, lets call it notification.

Comment 82

[u279076]

Attachment: Patch v4.1 [checked-in]

Comment 83

[u279076]

Comment on attachment 508040 [details] [diff] [review]
Patch v4.1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/f4209042c07c [default]

Backport patch coming up.

Comment 84

[u279076]

(In reply to comment #83)
> Backport patch coming up.

On second thought, since this test is not failing on branches, lets not touch it.  In fact, this test has not failed for 10 days on the branches.  We can address that separately, in a new bug, if/when it starts failing on branches.

Marking resolved fixed -- will mark verified after tomorrow's testrun.

Comment 85

[u279076]

Error started reappearing on the branches only on Feb 5th and is happening every day.  Reopening to investigate.

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

Comment 86

[u279076]

I've tried to reproduce this error locally and on the VM on qa-horus, unsuccessfully.  The only time I'm able to reproduce the failure is if I forcibly steal focus away from Firefox while the test is running.

Is it possible this is a focus issue and that something is stealing it from Firefox on this VM?  It might explain why we only see it recently on the Ubuntu 10.10 32-bit VM on qa-horus recently.

Henrik, please advise.

Comment 87

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

How did you run the test? With the testrun_all script? If not please do so.

Comment 88

[u279076]

(In reply to comment #87)
> How did you run the test? With the testrun_all script? If not please do so.

Ok, I can trigger the failure using testrun_all locally.  The only difference I can see between testrun_all and testrun_general is that the update tests are triggered first.  Are there any other differences? Is there a chance our update tests are causing the failure?

Comment 89

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

Sounds great! Next step would be to strip down the mozmill-tests repository to only those tests which are necessary to run before the password one, which could cause this problem. That's the first thing we have to check, before we could make assumptions about the update tests.

Comment 90

[u279076]

I've narrowed this down to a set of tests:

testBookmarks
testCookies
testFormManager
testSessionStore
testPasswordManager

Remove any of these and the test passes.  Tests must be run using testrun_all.py otherwise they pass.

So, it would seem that there is something happening between all of these tests which is causing the problem.  I have no idea how to debug this any further.

Henrik, do you have any suggestions?

Comment 91

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

I miss the tests inside those folders. Which of those are really necessary? Most of these folders have multiple test modules in it.

Comment 92

[u279076]

(In reply to comment #91)
> I miss the tests inside those folders. Which of those are really necessary?
> Most of these folders have multiple test modules in it.

I haven't broken those folders out test by test yet.  That will take me quite a bit more time.  I'll do that and report back in a few days.

Comment 93

[u279076]

Ok, I've narrowed it down to a couple of tests:

testSessionStore/testUndoTabFromContextMenu.js
testFormManager/testClearFormHistory.js
testPasswordManager/testPasswordNotSaved.js
(in that order)

Henrik, any recommendations on how I can debug this further?

Comment 94

[u279076]

This seems to be a focus issue.  We are failing to type the credentials in the form.  However, there is no dialog or context menus blocking focus.  I tried setting the form element to autofocus and even tried to force .focus() but we just can't seem to get focus here.

I'm wondering if there is something we could be doing to better detect and debug focus in Mozmill itself.

Comment 95

[u279076]

Ok, I've found a workaround...adding tabBrowser.closeAllTabs() to the end of the setupModule().  It would seem like testClearFormHistory (which has no teardownModule) is finishing in a dirty state.

Henrik, please advise.

Comment 96

[u279076]

As per discussion on IRC, I tried inserting a sleep(30000) before the controller.type().  I can't even manually get focus on the textboxes in the form.  So it would seem there is a focus issue going on here (I suspect something bad is happening in testClearFormHistory).

To test yourself on qa-horus:
1. Switch to the Ubuntu 10.10 32-bit VM
2. Open terminal to /mnt/hgfs/data/testing/ashughes
3. Run ./mozmill-automation/testrun_general.py --repository=./mozmill-tests-1.9.1/ --logfile=testrun.log ~/Desktop/Link\ to\ builds/shiretoko/firefox
4. Let the tests run until the login form appears
5. Try to click one of the textboxes and type

Result:
Cannot get focus of form elements.

Comment 97

[u279076]

Just a follow-up on what was discussed in IRC today.

We copied the Ubuntu 10.10 32-bit VM from qa-horus to qa-set to see if the bug is reproducible and here is what I found...

Ubuntu 10.10 32-bit on QA-Set: test passes
Ubuntu 10.10 32-bit on QA-Horus: test fails
Ubuntu 10.10 32-bit from QA-Horus on QA-Set: test fails

So, it would seem there is a "problem" with the VM from QA-Horus.  The obvious quick-fix here is to simply clone the QA-Set VM to replace the QA-Horus VM.  

Henrik, please advise.

Comment 98

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

This is scary, because I have installed a complete fresh VM for Ubuntu 10.10 some weeks back. Al, how did the upgrade happen on QA-Set? I would say you haven't installed a fresh 10.10?

For the easiest case I can copy over the Ubuntu machine from qa-set and add the cronjobs. I will do it right away, so we can check our todays test-run.

Comment 99

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

Machine has been copied over and I have started a testrun to update all machines to yesterdays build. Lets see the results.

Comment 100

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

Looks like it has been fixed the problem. Lets wait for another test-run today.

Comment 101

[Al Billings [:abillings - ex-MoCo]]

The previous VM was an upgrade from a 9.10 VM to 10.4 and then 10.10 as distro upgrades within linux. It was not a clean install (and most of our users aren't doing clean installs either).

Comment 102

[u279076]

This has not failed since cloning the VM from QA-Set -- marking RESOLVED FIXED.

I'll mark it verified if it's still passing next week.

Comment 103

[u279076]

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=All&platform=All&from=2011-02-17&to=2011-02-26&test=firefox%2FtestPasswordManager%2FtestPasswordNotSaved.js&func=testPasswordNotificationBar

This test is intermittent orange, failing 3 times in the last 10 days, only on the Ubuntu 10.10 32-bit VM.  What's our policy for dealing with intermittent orange tests?

Comment 104

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

Anthony, looks like we can close this bug again. The new Ubuntu machine has been fixed this.

Comment 105

[u279076]

Looks like this has not failed since Feb 25th -- re-resolving.

Comment 106

[u279076]

No failures in quite a while.  Marking VERIFIED.