957973 - Enhance waitForPageLoad() with more details for the load status in case of timeout

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andreea Matei [:AndreeaMatei]]

In bug 826693, we still have some tests that fail from time to time, the result being the page not being loaded: "controller.waitForPageLoad(): Timeout waiting for page loaded."

We need to add more details to the method so we can see what happens exactly in this situation.

Mozmill 2.0.3 clearly has improved the rate of this failure, but we still encounter it.

Comment 1

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

I would also suggest that we add which URL we were waiting for to be loaded. Also in case if the wait was successful we should add the URL.

Comment 2

[Sambuddha Basu [:sambuddhabasu1]]

Hi Henrik. I would like to work on this bug. Can you please assign it to me?

Comment 3

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

We have seen again a lot of waitForPageLoad issues recently, which are aren't sure what's causing those. So having this bug in the next release would be a great help.

Comment 4

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

The code which needs to be enhanced can be found here:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L986

We will not be able to include the information by simply adding it to the message string, given that it will be evaluated only when entering the assert.waitFor() call. Looks like we have to catch the exception, and re-throw it with the details added.

Comment 5

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

I will take it so that we can have it fixed for mozmill 2.0.6.

Comment 6

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

Attachment: Patch v1

This patch adds the requested details to the log messages of waitForPageLoad(). So we now know about the URI being loaded, and the latest readyState.

Cosmin, can you please test it with our mozmill tests on Windows and OS X? Thanks.

Comment 7

[Andrew Halberstadt [:ahal]]

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

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

::: mutt/mutt/tests/js/testUtils/testPageLoad.js
@@ +84,5 @@
> +  //   controller.waitForPageLoad(0);
> +
> +  //   throw new Error("controller.waitForPageLoad() not timed out for timeout=0.");
> +  // } catch (ex if ex isinstanceof errors.TimeoutError) {
> +  // }

Should probably delete this? The old version is still in the vcs.

Comment 8

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

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> > +  //   controller.waitForPageLoad(0);
> > +
> > +  //   throw new Error("controller.waitForPageLoad() not timed out for timeout=0.");
> > +  // } catch (ex if ex isinstanceof errors.TimeoutError) {
> > +  // }
> 
> Should probably delete this? The old version is still in the vcs.

I would leave this in. In a couple of cases we found that people were interested in fixing those issues. It's like a skip in the manifest, which we sadly cannot do here.

Waiting for the feedback from Cosmin before it gets landed.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

I had ran the mutt testpy twice, mutt testall once, testrun_remote and testrun_functional for Windows 8.1 x86 and osx 10.7.5 machines.
Everything ran great, it doesn't broke anything.
Thanks

Comment 10

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

https://github.com/mozilla/mozmill/commit/0ca6115f5952079be1a63ace839393000456467e (master)
https://github.com/mozilla/mozmill/commit/5c4f9ae38bbb70548bc5886f58adde404c648538 (hotfix-2.0)