Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seperate out bidi_session from session in fixtures.py #27734

Merged
merged 2 commits into from Feb 25, 2021

Conversation

@k7z45
Copy link
Contributor

@k7z45 k7z45 commented Feb 22, 2021

PR for #27687

Seperated out bidi_session from session in fixtures.py
Removed bidi markers and created bidi_session fixtures

Added handling in bidi_session and session start function
to disguish bidi_session and session so that async functions
can be awaited.

Modified tests to have better names.

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Feb 22, 2021

Removed bidi markers and created bidi_session fixtures

Add handling in bidi_session and session start function
to disguish bidi_session and session so that async functions
can be awaited.

Modify tests to have better names.
@k7z45 k7z45 force-pushed the k7z45:API branch from 0e3d3e1 to 898dfd2 Feb 22, 2021
@andreastt andreastt removed their request for review Feb 22, 2021
Copy link
Contributor

@jgraham jgraham left a comment

The main part of this looks reasonable. I'm somewhat nervous about all the tests that are basically testing the infrastructure itself and depend on everything running in source order.

Also delete some comments requiring specific order for test.
Added comment for first and last test to avoid breaking classic
webdriver session tests.
@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Feb 23, 2021

The main part of this looks reasonable. I'm somewhat nervous about all the tests that are basically testing the infrastructure itself and depend on everything running in source order.

Thanks! I removed an infrastructure changes and removed some comments so that they are not intended for infrastructure tests anymore. Kept the first and last test so that bidi tests do not impact classic tests.

@stephenmcgruer stephenmcgruer merged commit 8523985 into web-platform-tests:master Feb 25, 2021
19 of 22 checks passed
19 of 22 checks passed
update-pr-preview
Details
sink-task Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20210223.49 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
Copy link
Contributor

@whimboo whimboo left a comment

This looks way better! Thanks a lot. Nevertheless some more inline comments. And sorry for being late here, but I was off yesterday.

await session.websocket_transport.send("test_bidi_session_2")
await session.websocket_transport.close()
async def test_bidi_session_send(bidi_session):
await bidi_session.websocket_transport.send("test_bidi_session: send")

This comment has been minimized.

@whimboo

whimboo Feb 26, 2021
Contributor

It would be good to have a follow-up here for BiDiSession.send_command(). That way we won't have to expose the websocket_transport to all of the test files, and can then even build BiDiSession.send_session_command() on top of it.

@pytest.mark.capabilities({"acceptInsecureCerts": True})
async def test_bidi_session_3(session):
await session.websocket_transport.send("test_bidi_session_3")
async def test_bidi_session_with_different_capability(bidi_session):

This comment has been minimized.

@whimboo

whimboo Feb 26, 2021
Contributor

As mentioned in my reviews for the initial PR (and what @jgraham also referred to earlier in this PR) lets get an issue filed so we can make sure to have a single test for that. It would most likely need the new_session fixture, or maybe a new one for BiDiSession.

is_cur_bidi = isinstance(_current_session, webdriver.BidiSession)
# If there is a session with different capabilities active or the current session
# is of different type than the one we would like to create, end it now.
if _current_session is not None and (
(not _current_session.match(caps)) or
(is_cur_bidi != bidi)):
if is_cur_bidi:
await _current_session.end()
else:
_current_session.end()
_current_session = None
if _current_session is not None:
is_bidi = isinstance(_current_session, webdriver.BidiSession)
if is_bidi or caps != _current_session.requested_capabilities:
if is_bidi:
await _current_session.end()
else:
_current_session.end()
_current_session = None
Comment on lines 142 to +152

This comment has been minimized.

@whimboo

whimboo Feb 26, 2021
Contributor

This code is duplicated. Maybe we can just move it out to its own function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants