Closed Bug 1470533 Opened 6 years ago Closed 6 years ago

[wdspec] "New Session" tests must not use the global "new_session" fixture

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

Details

Attachments

(3 files)

Bug 1470533 - [wdspec] Add global "current_session" fixture.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470533 - [wdspec] Mark expected to fail new session tests on MacOS and Windows.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1470533 - [wdspec] Have "New Session" tests use a shared command fixture.
59 bytes, text/x-review-board-request
ato
: review+
Details 
Similar to all the other commands the tests for "New Session" should not use the global `new_session` fixture nor the high-level method of wdclient. Instead tests have to define their own shared method based on the transport layer of wdclient.

This bug blocks my work on bug 1470530, were we only want to create a new session if the capabilities are different. But that currently fails because the `new_session` tests are testing all kinds of behavior, and which should not be supported by that mentioned fixture.
Blocks: 1470530
Attachment #8987622 - Flags: review?(ato)
Attachment #8987623 - Flags: review?(ato)
Attachment #8987624 - Flags: review?(ato)
Comment on attachment 8987622 [details]
Bug 1470533 - [wdspec] Add global "current_session" fixture.

https://reviewboard.mozilla.org/r/252842/#review259588
Attachment #8987622 - Flags: review?(ato) → review+
Comment on attachment 8987623 [details]
Bug 1470533 - [wdspec] Mark expected to fail new session tests on MacOS and Windows.

https://reviewboard.mozilla.org/r/252844/#review259590
Attachment #8987623 - Flags: review?(ato) → review+
Comment on attachment 8987624 [details]
Bug 1470533 - [wdspec] Have "New Session" tests use a shared command fixture.

https://reviewboard.mozilla.org/r/252846/#review259592

A couple of points about the naming of new_session (it’s not really
creating a new session and it’s kwarg for deleting the existing one
is doubly confusing), but fundamentally I have no objection to
landing this once you’ve taken the issues into consideration.

::: commit-message-e9dcd:1
(Diff revision 1)
> +Bug 1470533 - [wdspec] "New Session" tests have to use a shared command fixture.

“Have "New Session" tests use a shared command fixture”?

::: testing/web-platform/tests/webdriver/tests/new_session/conftest.py:16
(Diff revision 1)
>  
>  def flatten(l):
>      return [item for x in l for item in x]
>  
> +
> +@pytest.fixture(name="new_session")

Does this overload the other new_session fixture?

::: testing/web-platform/tests/webdriver/tests/new_session/conftest.py:27
(Diff revision 1)
> +    :param delete_existing_session: Allows the fixture to delete an already
> +     created custom session before the new session is getting created. This
> +     is useful for tests which call this fixture multiple times within the
> +     same test.
> +    """
> +    custom_session = {}

Why dict?

It appears that you store the response value in custom_session["session"],
but couldn’t this be just as well be assigned directly to custom_session?

::: testing/web-platform/tests/webdriver/tests/new_session/conftest.py:36
(Diff revision 1)
> +    )
> +
> +    def _delete_session(session_id):
> +        transport.send("DELETE", "session/{}".format(session_id))
> +
> +    def new_session(body, delete_existing_session=False):

I find it a bit confusing that this will re-use the existing session
when its name is "new_session", implying it will be a, well, new
session.

::: testing/web-platform/tests/webdriver/tests/new_session/response.py:18
(Diff revision 1)
> +
> +
> +def test_capabilites(new_session, add_browser_capabilites):
> +    response, _ = new_session({"capabilities": {"alwaysMatch": add_browser_capabilites({})}})
> +    value = assert_success(response)
> +    assert isinstance(value["sessionId"], unicode)

basestring

::: testing/web-platform/tests/webdriver/tests/new_session/response.py:42
(Diff revision 1)
> +    assert isinstance(value["capabilities"]["browserName"], unicode)
> +    assert isinstance(value["capabilities"]["browserVersion"], unicode)

basestring
Attachment #8987624 - Flags: review?(ato) → review+
Comment on attachment 8987624 [details]
Bug 1470533 - [wdspec] Have "New Session" tests use a shared command fixture.

https://reviewboard.mozilla.org/r/252846/#review259592

> “Have "New Session" tests use a shared command fixture”?

Sure.

> Does this overload the other new_session fixture?

Yes, that is a feature of pytest that sub conftest.py files can re-define fixtures.

> Why dict?
> 
> It appears that you store the response value in custom_session["session"],
> but couldn’t this be just as well be assigned directly to custom_session?

No, it cannot be assigned directly because in this case you don't have access to the variable which is defined outside of the inner function, and you will get an error for accessing the variable before definition.

This is the only way to get the session data out of the inner function, and keep it for clean-up.

> I find it a bit confusing that this will re-use the existing session
> when its name is "new_session", implying it will be a, well, new
> session.

The only other option would be to store the session data globally like we do in the top conftest.py file. I'm happy to do this change.

> basestring

I didn't change that but I'm happy to make that change.
Comment on attachment 8987624 [details]
Bug 1470533 - [wdspec] Have "New Session" tests use a shared command fixture.

https://reviewboard.mozilla.org/r/252846/#review259592

> The only other option would be to store the session data globally like we do in the top conftest.py file. I'm happy to do this change.

Talked with Andreas via IRC and he is fine with just shipping this change.
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 dbb402fe4e96fe0f35153c6c42231e40d5fb827b -d e97d629e0b08: rebasing 469536:dbb402fe4e96 "Bug 1470533 - [wdspec] Add global "current_session" fixture. r=ato"
merging testing/web-platform/meta/MANIFEST.json
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/b06b88b75c75
[wdspec] Add global "current_session" fixture. r=ato
https://hg.mozilla.org/integration/autoland/rev/5138b18cec59
[wdspec] Mark expected to fail new session tests on MacOS and Windows. r=ato
https://hg.mozilla.org/integration/autoland/rev/c0e6991fd942
[wdspec] Have "New Session" tests use a shared command fixture. r=ato
https://hg.mozilla.org/mozilla-central/rev/b06b88b75c75
https://hg.mozilla.org/mozilla-central/rev/5138b18cec59
https://hg.mozilla.org/mozilla-central/rev/c0e6991fd942
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11971 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: