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

WebDriver Bidi tests with new Py3 WebSockets dependencies #27195

Merged
merged 9 commits into from Feb 18, 2021

Conversation

@k7z45
Copy link
Contributor

@k7z45 k7z45 commented Jan 15, 2021

Created a prototype involving WebDriver Bidi client according to [1].

The dependencies for the WebSockets lib[2] is added in third party lib. Added the dependency for pytest-asyncio[3] in this PR as well.

Later versions of pytest[4] and pluggy[5] are needed for pytest-asyncio and they are included here. Pluggy is also needed and it's already included from rebase.

The change includes:

A simple WebDriver Bidi capabilities test to demonstrate enabling WebSockets according to the protocol.
Upgraded pytest and pluggy third party dependencies
Added an async style test that enables WebSocket by directly specifying the corresponding capability.
Add BidiSession class in WebDriver client for use in fixture to conveniently create a Bidi Session and tests to demonstrate behavior between tests.
[1] #26015 (comment)
[2] https://github.com/aaugustin/websockets
[3] https://github.com/pytest-dev/pytest-asyncio
[4] https://github.com/pytest-dev/pytest
[5] https://github.com/pytest-dev/pluggy

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Jan 15, 2021

To run the test, I believe still need to do
pip install pytest-asyncio
locally. (This is no longer needed after adding pytest-asynio from untar)
Then
./wpt --py3 run --log-mach=- --channel=dev chrome webdriver/tests/new_session/websocket_url.py
and
./wpt --py3 run --log-mach=- --channel=dev chrome webdriver/tests/websockets/connect.py

Actually, use python3 wpt run ... instead.

Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Hi Shengfa, thanks for the PR! I've started to take a look at it, but a quick question:

The actual dependencies for the WebSockets lib[2] [...] are not included in this PR. They are installed locally using pip install.

Is this comment (from the description) still accurate? It looks like you vendored websockets into third_party and added it to localpaths.py so I wasn't sure.

webdriver/tests/support/fixtures.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/client.py Outdated Show resolved Hide resolved
@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Jan 18, 2021

Hi Shengfa, thanks for the PR! I've started to take a look at it, but a quick question:

The actual dependencies for the WebSockets lib[2] [...] are not included in this PR. They are installed locally using pip install.

Is this comment (from the description) still accurate? It looks like you vendored websockets into third_party and added it to localpaths.py so I wasn't sure.

Thanks for looking at it. That's not true anymore. I will modify the description tomorrow.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Jan 21, 2021

Is it possible to clean up the commits here to make reviewing easier? In particular it would be great to have a single initial commit that vendors in third party code, and then one or more subsequent commits with the additional changes.

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Jan 21, 2021

Is it possible to clean up the commits here to make reviewing easier? In particular it would be great to have a single initial commit that vendors in third party code, and then one or more subsequent commits with the additional changes.

Hi James,
I tried to do git rebase -i HEAD~16
with
pick 4b64d94 Upgrade pytest to 6.1.1 to be used with pytest-asyncio
squash ff59ff2 Remove tools/third_party/pytest for subtree add
squash bcc3874 Squashed 'tools/third_party/pytest/' content from commit 0ad20b533f
squash 0b4aec0 Squashed 'tools/third_party/websockets/' content from commit 139085fe26
squash 182e94d Add websockets to localpaths
squash fbedffb Squashed 'tools/third_party/iniconfig/' content from commit 4e20f6bcde
squash 0e7f71f Add iniconfig in localpaths,
unfortunately, I don't know how to resolve all the merge conflicts correctly.

@stephenmcgruer stephenmcgruer self-requested a review Jan 21, 2021
@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Jan 22, 2021

unfortunately, I don't know how to resolve all the merge conflicts correctly.

Trying to rebase a set of changes which include a subtree add and a merge (89e15e08325f8a8c4ce062477ecb8e5f6d49cdeb Merge branch 'master' into websockets) is difficult. It may be easier building the branch from scratch via cherry-picks (and re-doing the subtree add). Let me know if you need some assistance doing that.

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Jan 22, 2021

Hi James,
Could you clarify what commits you expect to see, please?
I am having trouble trying to squash the subtree add commits into one commit.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Jan 26, 2021

Baiscally I want all the commits that vendor in libraries first and then either a single commit with your changes or a number of logically seperate commits with your changes, with no merge commits except those from subtree. The use case is to be able to filter out all the vendored code when doing the review.

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Jan 26, 2021

Got it, thanks for the clarification.

@k7z45 k7z45 force-pushed the k7z45:websockets branch from 1840291 to 4cbf884 Jan 26, 2021
Copy link
Contributor

@jgraham jgraham left a comment

This is really nearly ready to merge. I'm very excited for this to be landed!

tools/webdriver/webdriver/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

LGTM, but (a) please wait for jgraham to also approve, and (b) can we rebase the commits before landing so that the review changes are squashed back into the appropriate original commit? (E.g. 102b879 would be squashed into 8a43e9f, etc)

@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Feb 10, 2021

LGTM, but (a) please wait for jgraham to also approve, and (b) can we rebase the commits before landing so that the review changes are squashed back into the appropriate original commit? (E.g. 102b879 would be squashed into 8a43e9f, etc)

Will do that once approved. Thanks for the reminder!

webdriver/tests/support/fixtures.py Show resolved Hide resolved
webdriver/tests/support/fixtures.py Show resolved Hide resolved
webdriver/tests/websockets/connect.py Outdated Show resolved Hide resolved
await session.websocket_transport.send("test_bidi_session_1")
await session.websocket_transport.close()

# bidi session following a bidi session with the same capabilities

This comment has been minimized.

@whimboo

whimboo Feb 11, 2021
Contributor

This makes the test dependent on a strict order, and test_bidi_session_1 being enabled. To get rid of this all the code should be put into a single test.

This comment has been minimized.

@k7z45

k7z45 Feb 11, 2021
Author Contributor

One of the reasons for the tests is testing the behavior from re-using websocket_transport between tests with/without capability change. I don't think I can achieve that with a single test.

This comment has been minimized.

@whimboo

whimboo Feb 12, 2021
Contributor

We might need a similar new_session fixture for BiDi so that you can create multiple new sessions with different capabilities within a single test.

webdriver/tests/websockets/connect.py Outdated Show resolved Hide resolved
@k7z45 k7z45 force-pushed the k7z45:websockets branch from f71f47c to 7fb0819 Feb 12, 2021
@k7z45
Copy link
Contributor Author

@k7z45 k7z45 commented Feb 12, 2021

Reverted the latest commit for dedicated bidi_session fixture but kept the new bidi test folder.
Rebased commits from comments into appropriate original commit.

Will open another issue for remaining opened conservations after merge for

  1. Creating a bidi_session fixture instead of existing bidi marker to reduce user test code setup for the marker.
  2. Re-use global current_session instead of creating another global bidi session
  3. We might need a similar new_session fixture for BiDi so that you can create multiple new sessions with different capabilities within a single test.

Copy link
Contributor

@whimboo whimboo left a comment

I'm ok with moving the remaining parts to a new issue / PR. @jgraham mind doing a review pass yourself?

@k7z45 k7z45 requested a review from jgraham Feb 17, 2021
Copy link
Contributor

@jgraham jgraham left a comment

OK, I'm happy for this to land to get things unblocked, but I agree that the proposed followups will create a nicer API.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Feb 18, 2021

I think think one needs to be landed by manual push to preserve the merge commits?

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Feb 18, 2021

I think think one needs to be landed by manual push to preserve the merge commits?

Yes, I believe so. I will own syncing with Shengfa to get this landed (I'm not sure what permissions we'll need, but I assume I have them and he might not...)

k7z45 added 9 commits Feb 5, 2021
Use git add -f tools/third_party/pytest to include version file.
This includes _version.py and pytest.egg-info folder.
Test establishing a Bidirectional Session capability support
Test connecting to WebDriver session using websockets
and send a message in async/await style
1. Added a BidiSession class in bidi.py under tools/webdriver/webdriver
that will preset capability for websocket connection.
Created async versions of start and end for the class to handle
establishing and closing websocket connection.
2. Modify session fixture to take an extra boolean argument "bidi"
for creating bidi session. Change the fixture to async and use
await when dealing with bidi session start/end.
3. Set event loop scope to session(assuming it would reuse the same
event loop for all tests) to avoid event loop is closed error when
closing websocket connection.
4. Added tests for different bidi/classic session creation scenario
and tested connect/send/close.
@k7z45 k7z45 force-pushed the k7z45:websockets branch from a397240 to 8ee4f2c Feb 18, 2021
@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Feb 18, 2021

I have manually pushed the two subtree adds (which generate merge commits and so cannot be landed via GitHub's UI). We believe the rest of the PR can be landed via GitHub's UI, so shengfa has rebased it and we're waiting for the CI to run.

@stephenmcgruer stephenmcgruer merged commit def808a into web-platform-tests:master Feb 18, 2021
39 of 42 checks passed
39 of 42 checks passed
update-pr-preview
Details
build-and-publish
Details
build-and-tag
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 #20210218.58 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 (tools/ unittests: Windows + Python 3.6) tools/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: Windows + Python 3.8) tools/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.6) tools/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.8) tools/ unittests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.6) tools/wpt/ tests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.8) tools/wpt/ tests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.6) tools/wpt/ tests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.8) tools/wpt/ tests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.6) tools/wptrunner/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.8) tools/wptrunner/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.6) tools/wptrunner/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.8) tools/wptrunner/ unittests: macOS + Python 3.8 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
resources/ tests (Python 3.6) Community-TC (pull_request)
Details
resources/ tests (Python 3.8) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
tools/ unittests (Python 3.8) 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
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