Closed Bug 1392274 Opened 3 years ago Closed 2 years ago

Create {forward, refresh, back} WDSpec tests

Categories

(Testing :: Marionette, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: automatedtester, Assigned: whimboo)

References

Details

Attachments

(3 files, 5 obsolete files)

This navigation tests are missing and would be really useful
David are you planning to move over (most of) the tests we currently have for Marionette? We already have a pretty good coverage.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> David are you planning to move over (most of) the tests we currently have
> for Marionette? We already have a pretty good coverage.

yup!
Priority: -- → P1
Hi David. Will you have the time to continue on those patches? This P1 bug hasn't gotten an update for 3 months.
Flags: needinfo?(dburns)
I havent had time to finish off the tests that require handlers for wptserve. I think we can do that as a follow up and that some are better than none for now.
Flags: needinfo?(dburns)
Comment on attachment 8899608 [details]
Bug 1392274 - Add Navigate Forward wdspec tests

https://reviewboard.mozilla.org/r/170914/#review218770

::: commit-message-2186f:4
(Diff revision 5)
> +Bug 1392274 - Add Navigate Forward wdspec tests r?whimboo
> +
> +These are the wdspec tests for testing the forward command in
> +https://w3c.github.io/webdriver/webdriver-spec.html#forward

How are you going with the removal of the Marionette unit tests, which are covered by wdspec now? I feel that completely covered tests should be removed immediately.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:12
(Diff revision 5)
> +
> +<title>Navigation by manipulating the browser history</title>
> +<script type="text/javascript">
> +function forward() {
> +  var stateObj = { foo: "bar" };
> +  history.pushState(stateObj, "", "navigation_pushstate_target.html");

I don't see this HTML file included. If that relies on the wptserv additions we might want to remove testing history manipulation for now, to not implement half-baked tests.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:25
(Diff revision 5)
> +    return session.transport.send("POST",
> +                                  "session/%s/forward" % session.session_id)
> +
> +# 9.4 Forward
> +
> +def test_no_browsing_context(session, create_window):

To be in sync with the naming of tests in other modules inside this folder lets make sure to use `test_forward_` as prefix for all subtests.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:44
(Diff revision 5)
> +    result = forward(session)
> +
> +    assert_error(result, "unexpected alert open")
> +
> +
> +def test_prompt_accept(new_session):

Please also add the subtest for `dismiss`.

Also given that each of the alert subtests is using a new session we should move those out into their own module to get around timeout failures.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:97
(Diff revision 5)
> +    assert_dialog_handled(session, "dismiss #3")
> +
> +
> +def test_no_history(session):
> +    response = forward(session)
> +    assert_success(response)

Maybe we should also assert that it's still the same URL.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:128
(Diff revision 5)
> +    title = session.title
> +    assert title == "page 3"
> +
> +
> +def test_history_pushstate(session):
> +

nit: please remove this empty line, and maybe quickly check the rest of the file.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:134
(Diff revision 5)
> +    session.url = push_state_doc
> +    session.find.css("#forward")[0].click()
> +
> +    # History has been injected but the page hasn't changed
> +    title = session.title
> +    assert title == "Navigation by manipulating the browser history"

But the URL should have changed, and we should check for the existence of #forward.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:137
(Diff revision 5)
> +    # History has been injected but the page hasn't changed
> +    title = session.title
> +    assert title == "Navigation by manipulating the browser history"
> +    session.back()
> +
> +    response = forward(session)

Does this now load the content of the target page? If yes, we should check that #forward is not present anymore.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:150
(Diff revision 5)
> +    session.back()
> +    assert "#1234" in session.title
> +
> +    response = forward(session)
> +    assert_success(response)
> +    assert "#5678" in session.title

This subtest should look like:

* no hash -> hash
* hash -> other hash
* hash -> no hash
Attachment #8899608 - Flags: review?(hskupin) → review-
Comment on attachment 8899610 [details]
Bug 1392274 - Update Web Platform Tests manifests and meta data

https://reviewboard.mozilla.org/r/170918/#review218776

This commit should be the last, and it misses the tests for forward and refresh.
Attachment #8899610 - Flags: review?(hskupin) → review-
Comment on attachment 8900153 [details]
Bug 1392274 - Add Navigate Back wdspec tests

https://reviewboard.mozilla.org/r/171544/#review218778

Beside the created issue lots of the comments from the forward commit also apply here.

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:97
(Diff revision 4)
> +
> +def test_no_history(session):
> +    response = back(session)
> +    assert_success(response)
> +
> +

The forward test had a couple of more tests. Can you at least make sure that we cover the same areas for both forward and backward?
Attachment #8900153 - Flags: review?(hskupin) → review-
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review218780

::: testing/web-platform/tests/webdriver/tests/navigation/refresh.py:35
(Diff revision 5)
> +    result = refresh(session)
> +
> +    assert_error(result, "unexpected alert open")
> +
> +
> +def test_prompt_accept(new_session):

Please also add the case for `dismiss`.

::: testing/web-platform/tests/webdriver/tests/navigation/refresh.py:100
(Diff revision 5)
> +
> +    result = refresh(session)
> +    assert_success(result)
> +
> +    response = session.find.css("#someDiv")
> +    assert response == []

Shouldn't that be a 'no such element' error?

::: testing/web-platform/tests/webdriver/tests/navigation/refresh.py:103
(Diff revision 5)
> +
> +    response = session.find.css("#someDiv")
> +    assert response == []
> +
> +
> +def test_frames(session, create_frame):

Maybe a better name is `test_refresh_switches_to_parent_browsing_context`
Attachment #8899609 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 8899610 [details]
> Bug 1392274 - Update Web Platform Tests manifests and meta data
> 
> https://reviewboard.mozilla.org/r/170918/#review218776
> 
> This commit should be the last, and it misses the tests for forward and
> refresh.

For the update please also run a try build first, so I can see that everything is passing/failing as expected.
Comment on attachment 8899608 [details]
Bug 1392274 - Add Navigate Forward wdspec tests

https://reviewboard.mozilla.org/r/170914/#review218830

::: commit-message-2186f:4
(Diff revision 5)
> +Bug 1392274 - Add Navigate Forward wdspec tests r?whimboo
> +
> +These are the wdspec tests for testing the forward command in
> +https://w3c.github.io/webdriver/webdriver-spec.html#forward

We should only remove tests when we have moved the other tests that this patch hasnt covered. The extra load this is adding is neglible and really not worth the effort of worrying about.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:25
(Diff revision 5)
> +    return session.transport.send("POST",
> +                                  "session/%s/forward" % session.session_id)
> +
> +# 9.4 Forward
> +
> +def test_no_browsing_context(session, create_window):

No other files have this naming convention
Version: Version 3 → unspecified
Attachment #8899610 - Attachment is obsolete: true
Attachment #8899610 - Flags: review?(hskupin)
Comment on attachment 8899608 [details]
Bug 1392274 - Add Navigate Forward wdspec tests

https://reviewboard.mozilla.org/r/170914/#review226734

r=me with the remaining issue fixed for the history manipulation test.

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:163
(Diff revisions 5 - 6)
>      session.url = push_state_doc
>      session.find.css("#forward")[0].click()
>  
>      # History has been injected but the page hasn't changed
>      title = session.title
> -    assert title == "Navigation by manipulating the browser history"
> +    assert title != "Navigation by manipulating the browser history"

I still think we should be better in what we check here. Just caring about the title only, would not recognize a URL change which leaves the same title.

The Marionette unit tests for that compare the URL because that gets changed, and at the same time tries to find a not yet existent element (title should also work here).

::: testing/web-platform/tests/webdriver/tests/navigation/forward.py:177
(Diff revisions 5 - 6)
> +    session.url = url("/common/blank.html")
>      session.url = url("/common/blank.html#1234")
>      session.url = url("/common/blank.html#5678")
>  
>      session.back()
>      assert "#1234" in session.title

Just curious why don't you check the URL instead?
Attachment #8899608 - Flags: review?(hskupin) → review+
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review218780

> Please also add the case for `dismiss`.

I still don't see this missing test.

> Shouldn't that be a 'no such element' error?

I don't see a fix neither, or an explanation if I was wrong with the assumption.
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review226742
Attachment #8899609 - Flags: review?(hskupin) → review-
Comment on attachment 8900153 [details]
Bug 1392274 - Add Navigate Back wdspec tests

https://reviewboard.mozilla.org/r/171544/#review218778

> The forward test had a couple of more tests. Can you at least make sure that we cover the same areas for both forward and backward?

I don't see that the missing tests have been added.
Comment on attachment 8900153 [details]
Bug 1392274 - Add Navigate Back wdspec tests

https://reviewboard.mozilla.org/r/171544/#review226746

This test module is perma failing on try. Please have a look.
Attachment #8900153 - Flags: review?(hskupin) → review-
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review226748

::: testing/web-platform/tests/webdriver/tests/navigation/refresh.py:102
(Diff revision 6)
> +    assert_success(result)
> +
> +    response = session.find.css("#someDiv")
> +    assert response == []
> +
> +

Oh, and I also miss the history navigation test here. A refresh should actually really load the requested page.
Comment on attachment 8950218 [details]
Bug 1392274 - Update Web Platform Tests manifests and meta data

https://reviewboard.mozilla.org/r/219474/#review226750

::: testing/web-platform/meta/MANIFEST.json:385129
(Diff revision 2)
> +   "webdriver/tests/navigation/back.py": [
> +    [
> +     "/webdriver/tests/navigation/back.py",
> +     {}
> +    ]
> +   ],

If you don't want to keep each individual commit atomic we can do an all update here, but personally I would prefer to update the manifest in each of the other three commits.
Attachment #8950218 - Flags: review?(hskupin) → review+
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review218780

> I don't see a fix neither, or an explanation if I was wrong with the assumption.

No because we are using `findElements` which returns an empty array if no elements are found
Comment on attachment 8950218 [details]
Bug 1392274 - Update Web Platform Tests manifests and meta data

https://reviewboard.mozilla.org/r/219474/#review226750

> If you don't want to keep each individual commit atomic we can do an all update here, but personally I would prefer to update the manifest in each of the other three commits.

I prefer it separate as it doesnt distract from the actual tests and is easier to find in a history.
Comment on attachment 8899609 [details]
Bug 1392274 - Add Refresh wdspec tests

https://reviewboard.mozilla.org/r/170916/#review228516
Attachment #8899609 - Flags: review?(hskupin) → review+
Comment on attachment 8950218 [details]
Bug 1392274 - Update Web Platform Tests manifests and meta data

https://reviewboard.mozilla.org/r/219474/#review226750

> I prefer it separate as it doesnt distract from the actual tests and is easier to find in a history.

Keep in mind that this way makes it harder to bisect issues. Here it's not that dramatic because the tests won't just be run as long as the manifest hasn't been updated.
Comment on attachment 8900153 [details]
Bug 1392274 - Add Navigate Back wdspec tests

https://reviewboard.mozilla.org/r/171544/#review228522

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:44
(Diff revision 7)
> +    result = back(session)
> +
> +    assert_error(result, "unexpected alert open")
> +
> +
> +def test_prompt_accept(new_session):

I just noticed that in all other test modules we use the `test_handle_prompt...` prefix. Maybe you can update all three test modules for that so we are in sync?

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:121
(Diff revision 7)
> +    assert_error(result, "unexpected alert open")
> +    assert_dialog_handled(session, "dismiss #3")
> +
> +
> +def test_no_history(session):
> +    response = back(session)

Why don't we need a new session here similar to the forward test?

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:147
(Diff revision 7)
> +    title = session.title
> +    assert title == "page 1"
> +
> +
> +def test_history_pushstate(session):
> +

nit: This empty line can be removed.

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:159
(Diff revision 7)
> +
> +    response = back(session)
> +    assert_success(response)
> +
> +    # Check we are just navigating history and not the pages
> +    title = session.title

Change that to check the URL similar to the forward test.

::: testing/web-platform/tests/webdriver/tests/navigation/back.py:160
(Diff revision 7)
> +    response = back(session)
> +    assert_success(response)
> +
> +    # Check we are just navigating history and not the pages
> +    title = session.title
> +    assert title == "Navigation by manipulating the browser history"

I miss test_hashes() as you have for forward.
Attachment #8900153 - Flags: review?(hskupin) → review-
Status: NEW → ASSIGNED
Comment on attachment 8899608 [details]
Bug 1392274 - Add Navigate Forward wdspec tests

https://reviewboard.mozilla.org/r/170914/#review228520

::: testing/web-platform/tests/webdriver/tests/forward/forward.py:2
(Diff revision 8)
>  from tests.support.inline import inline
> -from tests.support.asserts import assert_success
> +from tests.support.fixtures import create_dialog

Alerts and dialogs are not needed in this test module. It should only appear in `user_prompts.py`.

Also this should have caused ES lint failures if a try job would have been run for the changes.
Attachment #8899608 - Flags: review+ → review-
Comment on attachment 8900153 [details]
Bug 1392274 - Add Navigate Back wdspec tests

https://reviewboard.mozilla.org/r/171544/#review260274

::: testing/web-platform/tests/webdriver/tests/back/back.py:2
(Diff revision 8)
>  from tests.support.inline import inline
> -from tests.support.asserts import assert_success
> +from tests.support.fixtures import create_dialog

Same here as for the other patch. This would cause linter failures.
Attachment #8900153 - Flags: review?(hskupin) → review-
As discussed with David I will pick this up for completion. I will work on it once bug 1470530 landed maybe tomorrow.
Assignee: dburns → hskupin
Depends on: 1470530
It might be good to also wait for the refactored user prompt tests (bug 1470098) too, which would simply those tests a lot.
Depends on: 1470098
I continued to work on those tests and majorly refactored the 3 patches from David. For each of the commands we will get about 23 new tests including the user_prompt tests.

I'm not sure how many of our existent Marionette unit tests I should additionally include. But I think it is something we can do later. Also because for some of the tests more work is necessary, eg. delayed page loads.
David, could you please close your mozreview patch series so that is lesser confusing with my shortly be attached patches? Thanks.
Flags: needinfo?(dburns)
Attachment #8995104 - Flags: review?(ato)
Attachment #8995105 - Flags: review?(ato)
Attachment #8995106 - Flags: review?(ato)
Attachment #8899608 - Attachment is obsolete: true
Attachment #8899609 - Attachment is obsolete: true
Attachment #8900153 - Attachment is obsolete: true
Attachment #8950218 - Attachment is obsolete: true
Thanks.
Flags: needinfo?(dburns)
Comment on attachment 8995104 [details]
Bug 1392274 - [wdspec] Add tests for Forward navigation command.

https://reviewboard.mozilla.org/r/259580/#review266618

::: testing/web-platform/tests/webdriver/tests/forward/conftest.py:6
(Diff revision 1)
> +import pytest
> +
> +from webdriver.error import NoSuchWindowException
> +
> +
> +@pytest.fixture(name="session")

Shouldn’t this be called new_window or some such?

::: testing/web-platform/tests/webdriver/tests/forward/forward.py:84
(Diff revision 1)
> +    assert_success(response)
> +
> +    assert session.url != url_beforeunload
> +
> +
> +def test_hashes(session, url):

s/hashes/fragments/

::: testing/web-platform/tests/webdriver/tests/forward/forward.py:111
(Diff revision 1)
> +    assert session.url == test_pages[2]
> +
> +
> +def test_history_pushstate(session, url):
> +    pushstate_page = inline("""
> +      <script type="text/javascript">

"type" is not needed.
Attachment #8995104 - Flags: review?(ato) → review+
Comment on attachment 8995105 [details]
Bug 1392274 - [wdspec] Add tests for Back navigation command.

https://reviewboard.mozilla.org/r/259582/#review266622

::: testing/web-platform/tests/webdriver/tests/back/back.py:72
(Diff revision 1)
> +    assert_success(response)
> +
> +    assert session.url != url_beforeunload
> +
> +
> +def test_hashes(session, url):

s/hashes/fragments/
Attachment #8995105 - Flags: review?(ato) → review+
Comment on attachment 8995106 [details]
Bug 1392274 - [wdspec] Add tests for Refresh navigation command.

https://reviewboard.mozilla.org/r/259584/#review266624
Attachment #8995106 - Flags: review?(ato) → review+
Comment on attachment 8995104 [details]
Bug 1392274 - [wdspec] Add tests for Forward navigation command.

https://reviewboard.mozilla.org/r/259580/#review266618

> Shouldn’t this be called new_window or some such?

It's still a fixture which prepares the session. If we rename it, all of the test methods would have to be changed to make use of that new name. Having "new_window" which acts as `session` would look really wierd, eg. `new_window.find.css`.
Comment on attachment 8995104 [details]
Bug 1392274 - [wdspec] Add tests for Forward navigation command.

https://reviewboard.mozilla.org/r/259580/#review266618

> It's still a fixture which prepares the session. If we rename it, all of the test methods would have to be changed to make use of that new name. Having "new_window" which acts as `session` would look really wierd, eg. `new_window.find.css`.

Also note that this fixture override makes the behavior transparent to the tests. Otherwise for each test the `create_window` fixture would have to be used.
(In reply to Henrik Skupin (:whimboo) from comment #71)

> It's still a fixture which prepares the session. If we rename it,
> all of the test methods would have to be changed to make use of
> that new name. Having "new_window" which acts as `session` would
> look really wierd, eg. `new_window.find.css`.

I see what you mean, because fixtures are implicitly called it would
keep that name.  Just do what you think is best then.
Ok, so I would leave that for now. If it causes confusion we can always change it. Thanks.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7412ecd735e2
[wdspec] Add tests for Forward navigation command. r=ato
https://hg.mozilla.org/integration/autoland/rev/3e36cda9759b
[wdspec] Add tests for Back navigation command. r=ato
https://hg.mozilla.org/integration/autoland/rev/240e44df6f74
[wdspec] Add tests for Refresh navigation command. r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12209 for changes under testing/web-platform/tests
Flags: needinfo?(hskupin)
Upstream PR merged
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.