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.

Sign up for GitHub

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

Jump to bottom

BiDi script.callFunction #34231

Merged
merged 10 commits into from Jun 29, 2022
Merged

BiDi script.callFunction #34231

merged 10 commits into from Jun 29, 2022

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented May 27, 2022 •
edited

  • Add script.callFunction BiDi method.
  • Tests for happy case scenarios.
  • Tests for invalid params.

@sadym-chromium sadym-chromium force-pushed the script.callFunction branch 4 times, most recently from 92e6ffa to e3a62ed Compare June 7, 2022 14:52
@sadym-chromium
Copy link
Contributor Author

sadym-chromium commented Jun 10, 2022 •
edited

blocked by #34162

@sadym-chromium sadym-chromium changed the title script.callFunction BiDi script.callFunction Jun 10, 2022
@sadym-chromium sadym-chromium mentioned this pull request Jun 10, 2022
Closed
4 tasks
@sadym-chromium sadym-chromium force-pushed the script.callFunction branch 2 times, most recently from cb691fe to e7b2b2d Compare June 20, 2022 14:11
@sadym-chromium sadym-chromium marked this pull request as ready for review June 20, 2022 14:51
@sadym-chromium
Copy link
Contributor Author

@whimboo PTAL

juliandescottes
juliandescottes reviewed Jun 22, 2022
View reviewed changes
Copy link
Contributor

@juliandescottes juliandescottes 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 the PR Maksim, a first round of comments. I'll try to use the tests against an implementation in Firefox and will re-review.

webdriver/tests/bidi/script/call_function/call_function.py Show resolved Hide resolved
webdriver/tests/bidi/script/call_function/call_function.py Outdated Show resolved Hide resolved
webdriver/tests/bidi/script/call_function/call_function.py Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Jun 23, 2022

I'll try to use the tests against an implementation in Firefox and will re-review.

@juliandescottes do you mean you'll use the implementation to find if any of the tests are broken in non-obvious ways? That seems useful, but just want to ensure that actually making the tests pass in Firefox isn't blocking review of this PR.

@sadym-chromium
Copy link
Contributor Author

@juliandescottes any updates on the PR?

@juliandescottes
Copy link
Contributor

juliandescottes commented Jun 28, 2022 •
edited

I'll try to use the tests against an implementation in Firefox and will re-review.

@juliandescottes do you mean you'll use the implementation to find if any of the tests are broken in non-obvious ways? That seems useful, but just want to ensure that actually making the tests pass in Firefox isn't blocking review of this PR.

Yes, it's just meant to check that there aren't asserts which are going to be challenging to satisfy for other vendors. I did the same for the evaluate test, which helped identifying that asserting line/column in stacktraces was not going to work out.

@juliandescottes any updates on the PR?

Sorry I've been delayed a bit, will try to give feedback today.

@foolip
Copy link
Member

foolip commented Jun 28, 2022

I'll try to use the tests against an implementation in Firefox and will re-review.

@juliandescottes do you mean you'll use the implementation to find if any of the tests are broken in non-obvious ways? That seems useful, but just want to ensure that actually making the tests pass in Firefox isn't blocking review of this PR.

Yes, it's just meant to check that there aren't asserts which are going to be challenging to satisfy for other vendors. I did the same for the evaluate test, which helped identifying that asserting line/column in stacktraces was not going to work out.

Great, running the tests once to look for incorrect assumptions is very useful. If a test has regressed it's also reasonable to dig into what has happened. I just want to check that review of tests isn't tied to implementation progress, since that would become very difficult unless Chrome and Firefox are implementing the same features at the same time. (Coordinating roadmaps to some degree is a good idea for other reasons though.)

@juliandescottes
Copy link
Contributor

Great, running the tests once to look for incorrect assumptions is very useful. If a test has regressed it's also reasonable to dig into what has happened. I just want to check that review of tests isn't tied to implementation progress, since that would become very difficult unless Chrome and Firefox are implementing the same features at the same time. (Coordinating roadmaps to some degree is a good idea for other reasons though.)

I agree. I just happened to be starting the implementation of callFunction when this review came in so I thought it would be good to cross check it.

juliandescottes
juliandescottes approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

Thanks. That looks fine to me and passes with Firefox against a dummy implementation of callFunction. We can add more tests later on, would just be nice to have one for default values, see inline comment

tools/webdriver/webdriver/bidi/modules/script.py Show resolved Hide resolved
webdriver/tests/bidi/script/call_function/call_function.py Show resolved Hide resolved
@juliandescottes
Copy link
Contributor

@sadym-chromium Maybe we should start using a formatter for those tests? Not sure we have the tooling to enforce that, especially considering that tests can be contributed and synced from various repositories, but maybe we can just agree on one formatter and try to use it for our patches?

We usually use the Black formatter at mozilla. Do you use a formatter on your side?

@sadym-chromium
Copy link
Contributor Author

@sadym-chromium Maybe we should start using a formatter for those tests? Not sure we have the tooling to enforce that, especially considering that tests can be contributed and synced from various repositories, but maybe we can just agree on one formatter and try to use it for our patches?

We usually use the Black formatter at mozilla. Do you use a formatter on your side?

I use some IDE auto-formatter, but would be happy to adopt some other later on.

@sadym-chromium sadym-chromium merged commit e331d0c into web-platform-tests:master Jun 29, 2022
@sadym-chromium sadym-chromium deleted the script.callFunction branch June 29, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliandescottes juliandescottes juliandescottes approved these changes

@whimboo whimboo Awaiting requested review from whimboo

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

@DanielRyanSmith DanielRyanSmith Awaiting requested review from DanielRyanSmith

@foolip foolip Awaiting requested review from foolip

@jgraham jgraham Awaiting requested review from jgraham

@shs96c shs96c Awaiting requested review from shs96c

Assignees

@whimboo whimboo

@juliandescottes juliandescottes

Labels
infra webdriver webdriver-bidi wg-testing_browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sadym-chromium @foolip @juliandescottes @whimboo @wpt-pr-bot @moz-wptsync-bot @DanielRyanSmith