Implement page loading strategies

ASSIGNED
Assigned to

Status

Testing
Marionette
P1
normal
ASSIGNED
4 years ago
11 days ago

People

(Reporter: mdas, Assigned: whimboo)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

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

3 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

12 days 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

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

Comment 9

12 days 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

11 days 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

11 days ago
Attachment #8859572 - Flags: review?(ato)
Attachment #8859573 - Flags: review?(ato)

Comment 15

11 days 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

11 days 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

11 days 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

11 days 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
You need to log in before you can comment on or make changes to this bug.