Closed Bug 1470530 Opened 6 years ago Closed 6 years ago

[wdspec] Make "session" fixture to use a marker for capabilities, and only start a new session if capabilities are different

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1470530 - [wptrunner] Correct the warning to error option for pytest.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470530 - [wdspec] Allow parametrization of "session" fixture and remove global "new_session" fixture.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470530 - [wdspec] Always use the "create_dialog" and "create_window" fixtures.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470530 - [wdspec] Move "add_browser_capabilities" fixture under "new_session".
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470530 - [wdspec] Refactor "session" fixture for clean-up steps.
59 bytes, text/x-review-board-request
ato
: review+
Details 
Currently the `new_session` always restarts the session even if the requested capabilities from the next test are identical to the current session.

Limiting creating new session to only when it is really necessary will save us from having to restart Firefox that often. Especially I need this for bug 1470098 where I would like to parametrize the tests for user prompts.
Depends on: 1470533
Priority: -- → P3
Since bug 1470533 is fixed we now have a way better situation for the tests. Given that the `new_session` fixture no longer needs to accept the full body for the New Session command, we can reduce it to just the capabilities. 

But even more we can let the tests parametrize the capabilities, which the `session` fixture can retrieve, and directly return an active session. So the global `new_session` fixture is no longer necessary, and would not cause confusion with the `new_session` fixture of the `New Session` tests. Code will look like the following:

> @pytest.mark.parametrize("capabilities", [{"unhandledPromptBehavior": "accept"}])
> def test_handle_prompt_accept(session):

That code snippet will create a session with the `unhandledPromptBehavior` set to `accept`.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: [wdspec] new_session fixture should only start a new session if capabilities are different → [wdspec] Make "session" fixture parametrizable by tests and only start a new session if capabilities are different
Depends on: 1471629
Blocks: 1392274
Actually (thanks to Raphael and Dave for the info) we could even use a custom marker for the session capabilities, which would make it even more simpler, and especially distinguishable from parametrized tests. Here how it would look like:

> @pytest.mark.capabilities({"unhandledPromptBehavior": "accept"})
> def test_handle_prompt_accept(session):

For this feature we would need pytest 3.6.2, which I'm about to upgrade to on bug 1471629. The patch actually got already landed. So a patch for this bug will be up by tomorrow if nothing gets backed out.
The current patches work as is when pytest 3.6.2 is in use. I tested those locally by manually patching pytest 3.6.2.

Please note that I haven't updated the MANIFEST.json yet given that for each commit I might have to make changes anyway. So I will include those changes with the updated patches.

Once my patch on bug 1471629 got merged I will rebase this patch, and trigger a try build.
Summary: [wdspec] Make "session" fixture parametrizable by tests and only start a new session if capabilities are different → [wdspec] Make "session" fixture to use a marker for capabilities, and only start a new session if capabilities are different
The patch is now clearly ready for review. It's all passing on try.
Comment on attachment 8988595 [details]
Bug 1470530 - [wptrunner] Correct the warning to error option for pytest.

https://reviewboard.mozilla.org/r/253820/#review261288
Attachment #8988595 - Flags: review?(ato) → review+
Comment on attachment 8988596 [details]
Bug 1470530 - [wdspec] Allow parametrization of "session" fixture and remove global "new_session" fixture.

https://reviewboard.mozilla.org/r/253822/#review261292

Great patch!

::: testing/web-platform/tests/webdriver/tests/conftest.py:23
(Diff revision 3)
> +def pytest_generate_tests(metafunc):
> +    if "capabilities" in metafunc.fixturenames:
> +        marker = metafunc.definition.get_closest_marker(name="capabilities")
> +        if marker:
> +            metafunc.parametrize("capabilities", marker.args, ids=None)

Clever.
Attachment #8988596 - Flags: review?(ato) → review+
Comment on attachment 8988597 [details]
Bug 1470530 - [wdspec] Always use the "create_dialog" and "create_window" fixtures.

https://reviewboard.mozilla.org/r/253824/#review261296
Attachment #8988597 - Flags: review?(ato) → review+
Comment on attachment 8988598 [details]
Bug 1470530 - [wdspec] Move "add_browser_capabilities" fixture under "new_session".

https://reviewboard.mozilla.org/r/253826/#review261298
Attachment #8988598 - Flags: review?(ato) → review+
Comment on attachment 8988599 [details]
Bug 1470530 - [wdspec] Refactor "session" fixture for clean-up steps.

https://reviewboard.mozilla.org/r/253828/#review261302

This seems OK on the surface and I assume you have tested this in
practice.
Attachment #8988599 - Flags: review?(ato) → review+
Comment on attachment 8988599 [details]
Bug 1470530 - [wdspec] Refactor "session" fixture for clean-up steps.

https://reviewboard.mozilla.org/r/253828/#review261302

As you see it run all on try, and I also did a lot of runs locally. Especially for the user prompt tests which always will cause a new session to be created.
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 93a4e90da0352514bd79dc3292dfda1aad633794 -d b636a45b545e: rebasing 471010:93a4e90da035 "Bug 1470530 - [wptrunner] Correct the warning to error option for pytest. r=ato"
rebasing 471011:d24b8c864fc6 "Bug 1470530 - [wdspec] Allow parametrization of "session" fixture and remove global "new_session" fixture. r=ato"
merging testing/web-platform/meta/MANIFEST.json
rebasing 471012:d0c82b9f4dff "Bug 1470530 - [wdspec] Always use the "create_dialog" and "create_window" fixtures. r=ato"
merging testing/web-platform/meta/MANIFEST.json
rebasing 471013:fe6421d6695c "Bug 1470530 - [wdspec] Move "add_browser_capabilities" fixture under "new_session". r=ato"
merging testing/web-platform/meta/MANIFEST.json
merging testing/web-platform/tests/webdriver/tests/new_session/create_alwaysMatch.py
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4190bcbd4ef
[wptrunner] Correct the warning to error option for pytest. r=ato
https://hg.mozilla.org/integration/autoland/rev/1ec59d0783a5
[wdspec] Allow parametrization of "session" fixture and remove global "new_session" fixture. r=ato
https://hg.mozilla.org/integration/autoland/rev/2732ac33aac9
[wdspec] Always use the "create_dialog" and "create_window" fixtures. r=ato
https://hg.mozilla.org/integration/autoland/rev/bbccc02967d3
[wdspec] Move "add_browser_capabilities" fixture under "new_session". r=ato
https://hg.mozilla.org/integration/autoland/rev/ea72ff9a6e23
[wdspec] Refactor "session" fixture for clean-up steps. r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11977 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: