Closed Bug 1455568 Opened 6 years ago Closed 6 years ago

[wdspec] Separate out alerts/prompts tests into a separate test module

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(3 files)

Bug 1455568 - [wdspec] Refactor folder structure of tests by command.
59 bytes, text/x-review-board-request
ato
: review+
ato
: review+
Details
Bug 1455568 - [wdspec] Use shared command function for each WebDriver command.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1455568 - [wdspec] Separate out user prompts tests into their own module.
59 bytes, text/x-review-board-request
ato
: review+
Details 
A lot of existent wdspec tests run those alert/prompt tests as subtests, which actually causes a new session to be used and as result the browser is being restarted. We currently know that this can fail for Firefox; see bug 1449538. So having those tests in their own module we could easily mark them as skip/disabled for now.

But also for maintainability it would be better to have those tests separated out. Personally I don't see value in having to rewrite those tests over and over again for each command. Better would be to have some kind of fixture which could be used, and would make it much more convenient. Moving the tests out of other test modules will be the first step.
Blocks: 1429391
I'm not going to make that refactoring until tests are inappropriately failing. So I will add blockers to this bug.
Depends on: 1456799
Depends on: 1456996
Depends on: 1457054
Comment on attachment 8972004 [details]
Bug 1455568 - [wdspec] Use shared command function for each WebDriver command.

https://reviewboard.mozilla.org/r/240744/#review246434


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/web-platform/tests/webdriver/tests/element_click/stale.py:11
(Diff revision 1)
>  
>  
>  def click_element(session, element):
>      return session.transport.send(
> -        "POST", "/session/{session_id}/element/{element_id}/click".format(**{
> +        "POST", "/session/{session_id}/element/{element_id}/click".format(
>              "session_id": session.session_id,

Error: Unable to parse file [wpt: PARSE-FAILED]
Andreas, it would be great if you could review the first patch of this series so that we can agree on a common naming schema for the existing tests. Not sure if just `get.py` and similar are a very good choice.
Comment on attachment 8972004 [details]
Bug 1455568 - [wdspec] Use shared command function for each WebDriver command.

https://reviewboard.mozilla.org/r/240744/#review247388


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/web-platform/tests/webdriver/tests/element_click/stale.py:11
(Diff revision 2)
>  
>  
>  def click_element(session, element):
>      return session.transport.send(
> -        "POST", "/session/{session_id}/element/{element_id}/click".format(**{
> +        "POST", "/session/{session_id}/element/{element_id}/click".format(
>              "session_id": session.session_id,

Error: Unable to parse file [wpt: PARSE-FAILED]
Comment on attachment 8972003 [details]
Bug 1455568 - [wdspec] Refactor folder structure of tests by command.

https://reviewboard.mozilla.org/r/240742/#review247412

I approve of this change.  This is going to make it much easier to
remember where tests are located.
Attachment #8972003 - Flags: review?(ato) → review+
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Andreas, it would be great if you could review the first patch of this
> series so that we can agree on a common naming schema for the existing
> tests. Not sure if just `get.py` and similar are a very good choice.

The ‘Get’ command has since been renamed to ‘Navigate To’:
https://w3c.github.io/webdriver/#navigate-to

Maybe that will provide you with a better name?
Thank you for the review! Great that you liked it. 

(In reply to Andreas Tolfsen ‹:ato› from comment #12)
> The ‘Get’ command has since been renamed to ‘Navigate To’:
> https://w3c.github.io/webdriver/#navigate-to
> 
> Maybe that will provide you with a better name?

Sure, but my question was more related to all the other test files, which I named `get.py` like under each of the `get_element_*` sub folders.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #13)

> Thank you for the review! Great that you liked it.
>
> (In reply to Andreas Tolfsen ‹:ato› from comment #12)
>
> > The ‘Get’ command has since been renamed to ‘Navigate To’:
> > https://w3c.github.io/webdriver/#navigate-to
> >
> > Maybe that will provide you with a better name?
>
> Sure, but my question was more related to all the other
> test files, which I named `get.py` like under each of the
> `get_element_*` sub folders.

Oh I see.  I don’t know, at the moment this seems fine.  I suspect
the tests will grow to a point where it makes sense to split them
into multiple files anyway.
Flags: needinfo?(ato)
No longer blocks: 1449538
Comment on attachment 8972003 [details]
Bug 1455568 - [wdspec] Refactor folder structure of tests by command.

There have been made further changes to this patch. So please re-review. Thanks.
Attachment #8972003 - Flags: review?(ato)
Comment on attachment 8972003 [details]
Bug 1455568 - [wdspec] Refactor folder structure of tests by command.

https://reviewboard.mozilla.org/r/240742/#review248198

So for things like the Add Cookie command tests I guess you don’t
need a separate directory, as a single Python file will be equivalent
to a directory with the same name and an __init__.py file.

That would also resolve your naming conundrum.
Comment on attachment 8972004 [details]
Bug 1455568 - [wdspec] Use shared command function for each WebDriver command.

https://reviewboard.mozilla.org/r/240744/#review248200

::: testing/web-platform/tests/webdriver/tests/element_send_keys/form_controls.py:12
(Diff revision 4)
>      assert_success,
>  )
>  from tests.support.inline import inline
>  
>  
> -def element_send_keys(session, element, text):
> +def send_keys_to_element(session, element, text):

The command is actually called “Element Send Keys”.

::: testing/web-platform/tests/webdriver/tests/execute_script/json_serialize_windowproxy.py:25
(Diff revision 4)
> +    raw_json = assert_success(response)
> +
>      obj = json.loads(raw_json)

Hm I think this test might be double-unwrapping the response, but
this isn’t something you should worry about here.
Attachment #8972004 - Flags: review?(ato) → review+
Comment on attachment 8972005 [details]
Bug 1455568 - [wdspec] Separate out user prompts tests into their own module.

https://reviewboard.mozilla.org/r/240746/#review248202

This looks great!

::: commit-message-ca0e0:3
(Diff revision 4)
> +User prompt tests request new sessions and as such force the
> +browser to restart way more often. Extra delays due shutdown
> +and startup times of the browser are expected, and to better
> +handle timeouts and failures these tests should be located in
> +their own module.

This is the justification for the patch, but can you add a paragraph
about concretely what it is changing?

Since this patch is so large I suspect people will have a hard time
understanding what it does.
Attachment #8972005 - Flags: review?(ato) → review+
Attachment #8972003 - Flags: review?(ato) → review+
Comment on attachment 8972003 [details]
Bug 1455568 - [wdspec] Refactor folder structure of tests by command.

https://reviewboard.mozilla.org/r/240742/#review248198

No, it will cause at least problems with pytest. Each of the folders need a __init__.py file when the same filename gets re-used. It all works that way.
Comment on attachment 8972004 [details]
Bug 1455568 - [wdspec] Use shared command function for each WebDriver command.

https://reviewboard.mozilla.org/r/240744/#review248200

> Hm I think this test might be double-unwrapping the response, but
> this isn’t something you should worry about here.

No, `session.execute_script()` returns the value, while for our helper method we need the return value of `assert_success()`.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fb1a379745d
[wdspec] Refactor folder structure of tests by command. r=ato
https://hg.mozilla.org/integration/autoland/rev/a73b7f0983b0
[wdspec] Use shared command function for each WebDriver command. r=ato
https://hg.mozilla.org/integration/autoland/rev/9f13b6c1bfd7
[wdspec] Separate out user prompts tests into their own module. r=ato
https://hg.mozilla.org/mozilla-central/rev/5fb1a379745d
https://hg.mozilla.org/mozilla-central/rev/a73b7f0983b0
https://hg.mozilla.org/mozilla-central/rev/9f13b6c1bfd7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10938 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: