Closed Bug 937659 Opened 11 years ago Closed 7 years ago

Implement page loading strategies

Categories

(Testing :: Marionette Client and Harness, defect, P1)

defect

Tracking

(firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mdas, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

(Keywords: pi-marionette-client, pi-marionette-server, pi-marionette-spec, Whiteboard: [spec][17/04])

Attachments

(2 files)

We currently implement what is closest to 'normal' page loading in WebDriver, but we should allow the user to decide what strategy they want to use for page loading: https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#page-load-strategies-1

This spec mentions that we may select the strategy when we request a newSession, but I think that having a call like 'setPageLoadStrategy' would be useful if you want to explicitly set the strategy in the middle of a test. A use case for this would be where my test harness gives me a session that enforces 'normal' strategy, but I want to test the behaviour of a page as soon as its readyState is 'interactive', so instead of deleting and creating a new session, I can just dynamically change the strategy. Dburns, what do you think? Has this been discussed before?
Flags: needinfo?(dburns)
For B2G, we need the ability to test pages that are in an "interactive" state. Specifically when apps or the browser displays an about:neterror page, we want the ability to test functionality on that page. For this use case, it would be extremely useful to be able to set the page loading strategy on the fly, so that we don't have to create a new session each time we want to navigate to our error page.

Based on Malini's comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=936301#c3
(In reply to Malini Das [:mdas] from comment #0)
> We currently implement what is closest to 'normal' page loading in
> WebDriver, but we should allow the user to decide what strategy they want to
> use for page loading:
> https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#page-load-
> strategies-1
> 
> This spec mentions that we may select the strategy when we request a
> newSession, but I think that having a call like 'setPageLoadStrategy' would
> be useful if you want to explicitly set the strategy in the middle of a
> test. A use case for this would be where my test harness gives me a session
> that enforces 'normal' strategy, but I want to test the behaviour of a page
> as soon as its readyState is 'interactive', so instead of deleting and
> creating a new session, I can just dynamically change the strategy. Dburns,
> what do you think? Has this been discussed before?


This hasnt been discussed before but at the moment I don't really understand the use case for changing this strategy mid-test. 

For context, the main reason this was added in the first place was due to CDNs taking the longest to reply to requests (ironic isnt it). As you know, I am wary of adding anything API that can have a knock on effect (or allows people not to write deterministic tests), especially around navigation for later tests and think the advice given in https://bugzilla.mozilla.org/show_bug.cgi?id=936301#c3 is sound. 

I would be all for adding special cases for about:* pages so we don't get stuck in the interactive state for "special" pages.
Flags: needinfo?(dburns)
(In reply to David Burns :automatedtester from comment #3)
> (In reply to Malini Das [:mdas] from comment #0)
> I would be all for adding special cases for about:* pages so we don't get
> stuck in the interactive state for "special" pages.

I think that works in this case, what do you think mhenretty?
(In reply to Malini Das [:mdas] from comment #4)
> (In reply to David Burns :automatedtester from comment #3)
> > (In reply to Malini Das [:mdas] from comment #0)
> > I would be all for adding special cases for about:* pages so we don't get
> > stuck in the interactive state for "special" pages.
> 
> I think that works in this case, what do you think mhenretty?

That would be great. Thank you!
(In reply to Michael Henretty [:mhenretty] from comment #5)
> (In reply to Malini Das [:mdas] from comment #4)
> > (In reply to David Burns :automatedtester from comment #3)
> > > (In reply to Malini Das [:mdas] from comment #0)
> > > I would be all for adding special cases for about:* pages so we don't get
> > > stuck in the interactive state for "special" pages.
> > 
> > I think that works in this case, what do you think mhenretty?
> 
> That would be great. Thank you!

Okay, you can track that work here https://bugzilla.mozilla.org/show_bug.cgi?id=940084.

I'm leaving this bug open since we need to implement page loading strategy selection at *new session* as per spec requirements.
Blocks: 882186
Whiteboard: [spec]
Whiteboard: [spec]
Whiteboard: [marionette=1.0]
Summary: implement page loading strategies → Implement page loading strategies
Hardware: x86 → All
Priority: -- → P3
[mass] Setting priority
Priority: P3 → P1
Should be an easy thing to do now with having the page load listener in place. Taking.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Whiteboard: [marionette=1.0] → [spec][17/04]
I noticed that we do not correctly set the page load strategy if a null value is passed in. So I updated the code and also added a new unit test for it. But checking the spec I can see that we have to return an invalid argument error. That's what we also doing in Capabilities.match_(), but given that this code runs inside of newSession, the final exception is `SessionNotCreatedException` and not `InvalidArgumentException`. 

So I wonder if this is expected, or if we inappropriately wrap this exception into `SessionNotCreatedException`. At least I do not see another way of setting the page load strategy beside via capabilities, so this might be expected.
Flags: needinfo?(ato)
Also another webdriver spec related question most likely targeted for David:

> If the previous step completed by the session page load timeout being reached and the browser does not have an active user prompt, return error with error code timeout. 

Should we really return success if a user prompt is active? We really don't know why this prompt is open, and if the wanted page has been loaded at all. I feel that this causes an inconsistency. Maybe a different error is more appropriate?
Flags: needinfo?(dburns)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> I noticed that we do not correctly set the page load strategy if a null
> value is passed in. So I updated the code and also added a new unit test for
> it. But checking the spec I can see that we have to return an invalid
> argument error. That's what we also doing in Capabilities.match_(), but
> given that this code runs inside of newSession, the final exception is
> `SessionNotCreatedException` and not `InvalidArgumentException`. 

You will want to implement the matching of the pageLoadStrategy in geckodriver.  I’ve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1353727 to remove capabilities matching from Marionette.  Just try to work around it the best you can for the time being.

The correct error to return is ‘invalid argument’.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #10)
> Also another webdriver spec related question most likely targeted for David:
> 
> > If the previous step completed by the session page load timeout being reached and the browser does not have an active user prompt, return error with error code timeout. 
> 
> Should we really return success if a user prompt is active? We really don't
> know why this prompt is open, and if the wanted page has been loaded at all.
> I feel that this causes an inconsistency. Maybe a different error is more
> appropriate?

An ‘unexpected alert open’ error is returned by the next command request.  A user prompt may occur at any time and we generally don’t have a service watching for alerts to appear and abort commands if they do.  It’s consistent with WebDriver’s model to ignore the user prompt.
Flags: needinfo?(dburns)
Attachment #8859572 - Flags: review?(ato)
Attachment #8859573 - Flags: review?(ato)
Comment on attachment 8859572 [details]
Bug 937659 - Fix deserializing of page load strategy.

https://reviewboard.mozilla.org/r/131562/#review134422
Attachment #8859572 - Flags: review?(ato) → review+
Comment on attachment 8859573 [details]
Bug 937659 - Implement page load strategy.

https://reviewboard.mozilla.org/r/131564/#review134424

::: commit-message-b412f:1
(Diff revision 1)
> +Bug 937659 - Implement usage of page load strategy.

s/usage of//

::: commit-message-b412f:5
(Diff revision 1)
> +Bug 937659 - Implement usage of page load strategy.
> +
> +By using the page load strategy each navigation request has to return
> +when the page load has reached the expected document ready state, or
> +immediately if a strategy of `none` is set.

Refrain from Markdown in commit messages.
Attachment #8859573 - Flags: review?(ato) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e05e005afcc0 -d 368a60db302d: rebasing 390820:e05e005afcc0 "Bug 937659 - Fix deserializing of page load strategy. r=ato"
rebasing 390821:918cb61b59d8 "Bug 937659 - Implement page load strategy. r=ato" (tip)
merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/listener.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Bummer, this depends on the changes made on bug 1335778 which has been backed out yesterday. It means the landing of this patch is delayed.
Depends on: 1335778
Andreas, over on bug 1352132 you mentioned that `switchToWindow` should not wait for the contained tab to be finished loading. When I check `switchToFrame` now, I can see that we clearly wait for that.

Given that no navigation related checks are in the spec, I would assume that I can remove those?
Depends on: 1318351
Flags: needinfo?(ato)
No longer depends on: 1318351
(In reply to Henrik Skupin (:whimboo) from comment #22)
> Andreas, over on bug 1352132 you mentioned that `switchToWindow` should not
> wait for the contained tab to be finished loading. When I check
> `switchToFrame` now, I can see that we clearly wait for that.
> 
> Given that no navigation related checks are in the spec, I would assume that
> I can remove those?

It looks like we are, yes.  The specification doesn’t say that we should, so
we should probably remove that check:

    https://w3c.github.io/webdriver/webdriver-spec.html#switch-to-frame
Flags: needinfo?(ato)
Andreas, can you please review the latest changes? Try build doesn't show any failures.
Flags: needinfo?(ato)
Comment on attachment 8859572 [details]
Bug 937659 - Fix deserializing of page load strategy.

https://reviewboard.mozilla.org/r/131562/#review141042
Comment on attachment 8859573 [details]
Bug 937659 - Implement page load strategy.

https://reviewboard.mozilla.org/r/131564/#review141048

This is an excellent change.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:615
(Diff revision 4)
> +    def delete_session(self):
> +        if self.marionette.session is not None:
> +            self.marionette.delete_session()

This method appears unused?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:620
(Diff revision 4)
> +    def delete_session(self):
> +        if self.marionette.session is not None:
> +            self.marionette.delete_session()
> +
> +    def test_none(self):
> +        self.marionette.delete_session()

Did you mean to call self.delete_session() here?

Since this is called for every test, maybe it should be part of setUp?

::: testing/marionette/harness/marionette_harness/www/slow_resource.html:11
(Diff revision 4)
> +<html>
> +<head>
> +<title>Slow loading resource</title>
> +</head>
> +<body>
> +  <img src="/slow?delay=1" id="slow" />

Clever.
Flags: needinfo?(ato)
Comment on attachment 8859573 [details]
Bug 937659 - Implement page load strategy.

https://reviewboard.mozilla.org/r/131564/#review141048

> This method appears unused?

Fixed by the other mentioned issue.

> Did you mean to call self.delete_session() here?
> 
> Since this is called for every test, maybe it should be part of setUp?

Oh, that would make totally sense. Yes.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 42abb469bbdd -d 39ca824921c4: rebasing 394903:42abb469bbdd "Bug 937659 - Fix deserializing of page load strategy. r=ato"
rebasing 394904:0449339be446 "Bug 937659 - Implement page load strategy. r=ato" (tip)
merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/listener.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7ac732a7da75 -d 39ca824921c4: rebasing 394905:7ac732a7da75 "Bug 937659 - Fix deserializing of page load strategy. r=ato"
rebasing 394906:5530369399eb "Bug 937659 - Implement page load strategy. r=ato" (tip)
merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/listener.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like I have to wait for the next merge from autoland to mc before I can push this patch.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a7e2fe6328a
Fix deserializing of page load strategy. r=ato
https://hg.mozilla.org/integration/autoland/rev/9abc0a33e05b
Implement page load strategy. r=ato
https://hg.mozilla.org/mozilla-central/rev/2a7e2fe6328a
https://hg.mozilla.org/mozilla-central/rev/9abc0a33e05b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1364245
We discussed about a possible uplift of this bug last week, and agreed on that this will be useful. So I checked if everything can be applied cleanly, and only the commit 9abc0a33e05b needs a small fix, because the B2G removal changes are colliding.

I pushed a try build for verification:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8bb10accf95b5dfa9031c9e234392405ca5e41
Everything looks fine for the try build on mozilla-beta. So please use the following changesets from the try build for grafting:

86d943f7656d HS Bug 1364245 - Extend delay for slow loading resource testcase to prevent intermittent. r=ato
22652a3af136 HS Bug 937659 - Implement page load strategy. r=ato
f7aa6d871c5d HS Bug 937659 - Fix deserializing of page load strategy. r=ato

For 22652a3af136 I had to fix a minor merge conflict.

Thanks.
Whiteboard: [spec][17/04] → [spec][17/04][checkin-needed-beta]
(In reply to Henrik Skupin (:whimboo) from comment #42)
> Everything looks fine for the try build on mozilla-beta. So please use the
> following changesets from the try build for grafting:
> 
> 86d943f7656d HS Bug 1364245 - Extend delay for slow loading resource
> testcase to prevent intermittent. r=ato
> 22652a3af136 HS Bug 937659 - Implement page load strategy. r=ato
> f7aa6d871c5d HS Bug 937659 - Fix deserializing of page load strategy. r=ato
> 
> For 22652a3af136 I had to fix a minor merge conflict.
> 
> Thanks.

has merge conflicts in beta when trying to apply the first patch


patching file testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py
Hunk #1 FAILED at 625
1 out of 1 hunks FAILED -- saving rejects to file testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py.rej
unable to find 'testing/marionette/harness/marionette_harness/www/slow_resource.html' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file testing/marionette/harness/marionette_harness/www/slow_resource.html.rej
abort: patch failed to apply
Flags: needinfo?(hskupin)
Carsten, try is showing the commits in reversed order. The above listed ones were just a help for the person who is doing the uplift. Sorry that I didn't put them in the correct order. So starting from bottom up should work fine.
Flags: needinfo?(hskupin) → needinfo?(cbook)
Flags: needinfo?(cbook)
Whiteboard: [spec][17/04][checkin-needed-beta] → [spec][17/04]
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.