Skip to content

Allow running testharness/testdriver/reftests in servodriver#34550

Merged
jdm merged 9 commits intoservo:mainfrom
jdm:servodriver-revival
Dec 11, 2024
Merged

Allow running testharness/testdriver/reftests in servodriver#34550
jdm merged 9 commits intoservo:mainfrom
jdm:servodriver-revival

Conversation

@jdm
Copy link
Copy Markdown
Member

@jdm jdm commented Dec 10, 2024

These changes remove all of the custom integration code for the servodriver WPT executor, making it a tiny wrapper around the upstream webdriver executors, which should improve maintainability going forward. This PR also implements support for missing webdriver commands that are sent by the webdriver test executor, and extends the JS->webdriver serialization to cover arbitrary JS objects which is required by the integration with testharnessreport.js.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Can't run WPT test suite with servodriver #34523
  • These changes do not require tests because we don't use this test harness in CI yet. They were tested manually against tests/wpt/mozilla/tests and tests/wpt/tests/cookie with --product servodriver.

@jdm jdm requested a review from gterzian as a code owner December 10, 2024 03:23
@jdm jdm force-pushed the servodriver-revival branch 4 times, most recently from c94e264 to 247c1cf Compare December 10, 2024 07:34
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 10, 2024

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Dec 10, 2024 •
edited
Loading

@jdm Thanks for the help! I'm really glad tests require testdriver.js can run again.
Do you think it's possible merge servo and servodriver into one? Because @mrobinson mentioned it's better we can run WPT workflow with the same executor in yesterday's triage meeting.

@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Dec 10, 2024

@jdm Thanks for the help! I'm really glad tests require testdriver.js can run again. Do you think it's possible merge servo and servodriver into one? Because @mrobinson mentioned it's better we can run WPT workflow with the same executor in yesterday's triage meeting.

It's true that I think it'd be better to have one harness, but I'm happy if we move toward that situation gradually as the webdriver version of the harness improves.

wusyong
wusyong reviewed Dec 10, 2024
View reviewed changes
@jdm
Copy link
Copy Markdown
Member Author

jdm commented Dec 10, 2024

@jdm Thanks for the help! I'm really glad tests require testdriver.js can run again. Do you think it's possible merge servo and servodriver into one? Because @mrobinson mentioned it's better we can run WPT workflow with the same executor in yesterday's triage meeting.

I do think it's a realistic goal. Right now it would mean a regression in developer ergonomics, since the --debugger flag does not launch a debugger with the servodriver harness.

@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 10, 2024

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@jdm jdm force-pushed the servodriver-revival branch from a3e3193 to 856b79e Compare December 10, 2024 18:59
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 10, 2024

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@jdm
Copy link
Copy Markdown
Member Author

jdm commented Dec 10, 2024 

error: patch failed: tools/wptrunner/wptrunner/browsers/servodriver.py:9
error: tools/wptrunner/wptrunner/browsers/servodriver.py: patch does not apply
error: patch failed: tools/wptrunner/wptrunner/executors/executorservodriver.py:5
error: tools/wptrunner/wptrunner/executors/executorservodriver.py: patch does not apply
error: patch failed: tools/wptrunner/wptrunner/testharnessreport-servodriver.js:1
error: tools/wptrunner/wptrunner/testharnessreport-servodriver.js: patch does not apply

Not a good sign.

@jdm jdm mentioned this pull request Dec 10, 2024
Merged
@wusyong wusyong self-requested a review December 11, 2024 07:56
jdm added 7 commits December 11, 2024 12:41
@jdm
Make servodriver a thin wrapper over the base webdriver browser/execu…
5c53366 
…tor classes.

Signed-off-by: Josh Matthews <[email protected]>
@jdm
Make ServoWebDriverRefTestExecutor a thin shell over the webdriver re…
abed239 
…ftest executor.

Signed-off-by: Josh Matthews <[email protected]>
@jdm
Wait for the initial load to complete when opening a new tab via webd…
69d8878 
…river.

Signed-off-by: Josh Matthews <[email protected]>
@jdm
Remove assumption of a single tab from the webdriver server.
d5381f6 
Signed-off-by: Josh Matthews <[email protected]>
@jdm
Serialize all keys of JS objects when converting to webdriver values.
8463fbe 
Signed-off-by: Josh Matthews <[email protected]>
@jdm
Formatting.
bed87b8 
Signed-off-by: Josh Matthews <[email protected]>
@jdm
Cleanup, docs, etc.
58ca86f 
Signed-off-by: Josh Matthews <[email protected]>
@jdm jdm force-pushed the servodriver-revival branch from 856b79e to 1157661 Compare December 11, 2024 17:41
@jdm jdm enabled auto-merge December 11, 2024 17:41
@jdm jdm disabled auto-merge December 11, 2024 17:41
@servo-wpt-sync servo-wpt-sync mentioned this pull request Dec 11, 2024
Merged
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 11, 2024

🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#49642) with upstreamable changes.

@jdm jdm force-pushed the servodriver-revival branch from 1157661 to 0a05304 Compare December 11, 2024 18:36
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 11, 2024

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#49642).

1 similar comment
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 11, 2024

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#49642).

@jdm
Fix flake8 errors.
052c6ad 
Signed-off-by: Josh Matthews <[email protected]>
@jdm jdm force-pushed the servodriver-revival branch from 40b481a to 052c6ad Compare December 11, 2024 18:49
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 11, 2024

📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#49642).

@jdm jdm enabled auto-merge December 11, 2024 19:18
@jdm jdm added this pull request to the merge queue Dec 11, 2024
Merged via the queue into servo:main with commit 7b16070 Dec 11, 2024
@jdm jdm deleted the servodriver-revival branch December 11, 2024 20:12
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Dec 11, 2024

⛔ Failed to properly merge the upstream pull request (web-platform-tests/wpt#49642). Please address any CI issues and try to merge manually.

@wusyong wusyong mentioned this pull request Dec 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gterzian gterzian Awaiting requested review from gterzian

@wusyong wusyong Awaiting requested review from wusyong

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't run WPT test suite with servodriver

4 participants

@jdm @servo-wpt-sync @wusyong @mrobinson