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

Test automation for custom handlers #35792

Conversation

javifernandez
Copy link
Contributor

This commit implements the actions and protocol to use a new webdriver extension command to allow testing automation of the registerProtocolHandler method.

Additionally, this change also adds new tests which are automated versions of the manual tests already defined for the registerProtocolHandler method.

@wpt-pr-bot wpt-pr-bot added html infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run labels Sep 6, 2022
@javifernandez
Copy link
Contributor Author

javifernandez commented Sep 6, 2022

Since this change touches both, 'resources' and 'tools' folders it'd required an RFC proposal to be discussed and approved. See web-platform-tests/rfcs#121 for details.

T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Feb 4, 2023
The PR#8267 [1] for the HTML spec introduced a new WebDriver extension
command to allow testing automation for the registerProtocolHandler
method of the Custom Handlers API.

This CL adds support in ChromeDriver for such extension command. See
also the Web Platform Test PR [2] and RFC [3] for further details.

The extension command relies on a new CDP command implemented in a different CL [4].

[1] whatwg/html#8267
[2] web-platform-tests/wpt#35792
[3] web-platform-tests/rfcs#121
[4] https://crrev.com/c/4203529

Bug: 1359103
Change-Id: Ifd4385815ab30a40f46c2f083dc9604024060613
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200660
Reviewed-by: Vladimir Nechaev <nechaev@chromium.org>
Commit-Queue: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1100968}
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, this seems like the right approach to me!

Any reason not to get rid of the manual tests at the same time?

@javifernandez
Copy link
Contributor Author

Thanks for working on this, this seems like the right approach to me!

Any reason not to get rid of the manual tests at the same time?

I thought about it, tbh. They might be useful to try the codeapath of the Permission logic and user prompt. Additionally, they could be useful for the browser that implement the Custom Handlers API but not the WebDriver command (eg Firefox). Is this enough to keep all the manual tests ? I'm not sure; I trust your judgement on this regard.

@javifernandez javifernandez requested a review from a team as a code owner February 16, 2023 21:59
@annevk
Copy link
Member

annevk commented Feb 17, 2023

I guess that's fair. I worry a bit that they might not get maintained over time. I guess that would be easy enough to spot with git and we can make a call at that time what to do. And a lot of logic is shared which should reduce that issue as well.

This new WebDriver extension command allows to registerProtocolHandler
method of the HTML API to operate under an automatic mode.

This PR defines also new automated tests for the registerProtocolHandler
based on the manual tests that we alaready have.
@javifernandez javifernandez force-pushed the test-automation-for-custom-handlers branch from e57b2f4 to 4cee442 Compare November 14, 2023 16:20
@javifernandez
Copy link
Contributor Author

@annevk could you take another look at this PR ? Perhaps we can discuss again about what to do with the manual tests; I think they still have some value, since they test a different codepath and definitively the only option for browsers that don't implement the new WebDriver extension command (eg, Firefox).

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks okay. I didn't review in depth. As mentioned in my prior comment I'm happy to keep the manual tests for now and see what happens.

@javifernandez
Copy link
Contributor Author

@domenic perhaps you could rubber-stamp the PR, given that these tests are designed for the registerProtocolHandler method of the HTML spec.

@javifernandez
Copy link
Contributor Author

@jgraham your input would be interesting as well, given that Gecko is the other engine that implements the registerProtocolHandler method.

@domenic
Copy link
Member

domenic commented Nov 22, 2023

I'm sorry, I don't have time to review this PR. I see you already have an approval.

@javifernandez javifernandez merged commit 6004504 into web-platform-tests:master Nov 28, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants