604832 - Fix element declarations and usage in tests to avoid bad document references

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

The testStartStopPBMode.js test module fails on slower boxes like the buildbot ones. The reason is that we do not wait for page load but for the elements on the page. The timeout in such a case is only 5s instead of 30s for waitForPageLoad.

The following patch will change that and always wait for the page loaded. Also I have removed the unnecessary elements check right after the page load. We test those elements later in the test.

Comment 1

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

Attachment: Patch v1

Comment 2

[Aaron Train [:aaronmt]]

Comment on attachment 483684 [details] [diff] [review]
Patch v1

>   for (var i = 0; i < LOCAL_TEST_PAGES.length; i++) {
>+    controller.waitForPageLoad(controller.tabs.getTab(i));
>+
>     var elem = new elementslib.ID(controller.tabs.getTab(i), LOCAL_TEST_PAGES[i].id);
>     controller.waitForElement(elem, TIMEOUT);

Looks fine, but if we wait for page load, there's no reason to wait for elements if the page is done loading. If you want to adjust that before you land that's fine, or do it later. r+ anyways

Comment 3

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

The problem here is much broader as I have thought. One issue was that we weren't able to detect the page load in a background tab correctly. This will be fixed by my patch on bug 604878. But the other problem we have and why a lot of our tests are failing is the fact that elements are getting declared and reused at a later time. Especially if a page load is in between, which even loads the same page again, we fail because the underlying document has been changed and function calls like the following will fail:

var elem = elementslib.ID(controller.tab.activeTab, "id");
controller.open("xyz");
controller.waitForPageLoad();
controller.click(elem);

The solution for now is to move the element declaration right before its usage. With that change I was able to fix more than 15 failing tests (one fix sometimes brought up another failure).

The big question here is, which should be discussed on another bug: Is our handling of XULDocuments and HTMLDocuments correct. Personally I see a big flaw which breaks us in several areas. I will file that bug in the next minutes.

Comment 4

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

Attachment: Patch v1

Comment 5

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

Attachment: Patch v1.1

I forgot about the Back and Forward test. All the tests which are affected by the waitForPageLoad change should be fixed with those changes.

Comment 6

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

(In reply to comment #3)
> The big question here is, which should be discussed on another bug: Is our
> handling of XULDocuments and HTMLDocuments correct. Personally I see a big flaw
> which breaks us in several areas. I will file that bug in the next minutes.

Thinking more about, yes it is correct. When elements are created from a content document and the page gets reloaded, the element will have to be recreated. The reason is that the document gets replaced and old references are not valid anymore. We should always take this into account.

Comment 7

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

Attachment: Patch v1.2

Ok, we need one more revision here. As I got the information we can also use waitForPageLoad for bf-cache served pages. We would simply have to listen to "pageshow" events.

Comment 8

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

Attachment: Patch v1.3

The pageshow listener had to be removed because it caused a regression. It needs further investigation for an upcoming release. That means for pages served from the cache we have to wait for the element. I also had to fix testDisabledPermissions.js which wasn't using the tabBrowser.openTab function and failed with Minefield due to the tab animation.

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Re: recreating elements, the move to widget classes may make this easy.  I'd like to see what the speed is if we always lookup every time, for one, which eliminates this problem entirely.  Failing that, we can put in an "invalidate" type method that tips off the class to relookup after a page load.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 483924 [details] [diff] [review]
Patch v1.3

Patch looks fine. r+

Comment 11

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

I will land that patch after checking that we do not regress anything for the last release

Comment 12

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

We can't check in this patch as given by the whiteboard. We really have to wait for the first builds of Mozmill 1.5.1.

Comment 13

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

(In reply to comment #12)
> We can't check in this patch as given by the whiteboard. We really have to wait
> for the first builds of Mozmill 1.5.1.

Sorry my fault. Lets keep the checkin-needed keyword this time. We will have to find a better solution for the future.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

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

Comment 15

[Aaron Train [:aaronmt]]

Attachment: testPasswordSavedAndDeleted (default) [missing]

It looks like one was missing which is failing on 1.5.1RC with a timeout exceed as a result. This patch fixes that timeout.

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Also, seeing a number of element not found and waitForElement timeout failures on branches after going to 1.5.1. 

I assume that reflects a need to backport these changes?

Comment 17

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

Yes, we have to backport all of them.

Comment 18

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

Comment on attachment 485159 [details] [diff] [review]
testPasswordSavedAndDeleted (default) [missing]

Please don't redeclare the variables. A simple assignment will be perfect. I will include this patch for the backport. Thanks for spotting it.

Comment 19

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

Attachment: Backport v1 (1.9.2)

This is the backport patch for 1.9.2. I will now check the one for 1.9.1.

Comment 20

[Aaron Train [:aaronmt]]

Attachment: Follow-up fixes for default branch [checked-in]

Comment 21

[Aaron Train [:aaronmt]]

Comment on attachment 485283 [details] [diff] [review]
Backport v1 (1.9.2)

Patch looks great, thanks. r+

Comment 22

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

Attachment: Backport v1 (1.9.1)

Comment 23

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

Comment on attachment 485284 [details] [diff] [review]
Follow-up fixes for default branch [checked-in]

Looks good. thanks.

Comment 24

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

Comment on attachment 485284 [details] [diff] [review]
Follow-up fixes for default branch [checked-in]

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

Comment 25

[Aaron Train [:aaronmt]]

Comment on attachment 485288 [details] [diff] [review]
Backport v1 (1.9.1)

Great, r+

Comment 26

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

Landed backport patches:
http://hg.mozilla.org/qa/mozmill-tests/rev/77963e6b0d86 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/7fe53b44325f (1.9.1)

I don't think that we will have to revert to 1.5. I will mark this bug as fixed.

Comment 27

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

This bug has been fixed mostly all of those instances. Lets file new bugs for remaining issues.

Comment 28

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.