Implement page loading strategies

RESOLVED FIXED in Firefox 54

Status

Testing
Marionette
P1
normal
RESOLVED FIXED
4 years ago
28 days ago

People

(Reporter: mdas, Assigned: whimboo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {ateam-marionette-client, ateam-marionette-server, ateam-marionette-spec})

Trunk
mozilla55
ateam-marionette-client, ateam-marionette-server, ateam-marionette-spec
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [spec][17/04], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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)
Blocks: 721859
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
Duplicate of this bug: 936301
(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.

Updated

4 years ago
Blocks: 882186
Whiteboard: [spec]
Keywords: ateam-marionette-spec
Whiteboard: [spec]
Keywords: ateam-marionette-client, ateam-marionette-server
Whiteboard: [marionette=1.0]
Summary: implement page loading strategies → Implement page loading strategies
Hardware: x86 → All
Priority: -- → P3
[mass] Setting priority
Priority: P3 → P1
(Assignee)

Comment 8

2 months ago
Should be an easy thing to do now with having the page load listener in place. Taking.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Updated

2 months ago
Whiteboard: [marionette=1.0] → [spec][17/04]
(Assignee)

Comment 9

2 months ago
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)
(Assignee)

Comment 10

2 months ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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)
(Assignee)

Updated

2 months ago
Attachment #8859572 - Flags: review?(ato)
Attachment #8859573 - Flags: review?(ato)

Comment 15

2 months ago
mozreview-review
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 16

2 months ago
mozreview-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+
Comment hidden (mozreview-request)

Comment 18

2 months ago
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)
(Assignee)

Comment 19

2 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

2 months ago
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)
(Assignee)

Updated

2 months ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 months ago
Andreas, can you please review the latest changes? Try build doesn't show any failures.
Flags: needinfo?(ato)

Comment 27

2 months ago
mozreview-review
Comment on attachment 8859572 [details]
Bug 937659 - Fix deserializing of page load strategy.

https://reviewboard.mozilla.org/r/131562/#review141042

Comment 28

2 months ago
mozreview-review
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)
(Assignee)

Comment 29

2 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

2 months ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

2 months ago
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)
(Assignee)

Comment 36

2 months ago
Looks like I have to wait for the next merge from autoland to mc before I can push this patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

a month ago
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

Comment 40

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a7e2fe6328a
https://hg.mozilla.org/mozilla-central/rev/9abc0a33e05b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

a month ago
Depends on: 1364245
(Assignee)

Comment 41

a month ago
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
status-firefox54: --- → ?
(Assignee)

Comment 42

a month ago
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

Updated

a month ago
Flags: needinfo?(hskupin)
(Assignee)

Comment 44

a month ago
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)

Comment 45

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c7475dd59d76
https://hg.mozilla.org/releases/mozilla-beta/rev/780defc84a89
status-firefox54: ? → fixed

Updated

a month ago
Flags: needinfo?(cbook)
Whiteboard: [spec][17/04][checkin-needed-beta] → [spec][17/04]
Depends on: 1368227
You need to log in before you can comment on or make changes to this bug.