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

script: add missing serialization tests: iterator, generator, proxy #40814

Merged
merged 4 commits into from
Jul 5, 2023
Merged

script: add missing serialization tests: iterator, generator, proxy #40814

merged 4 commits into from
Jul 5, 2023

Conversation

thiagowfx
Copy link
Member

@thiagowfx thiagowfx commented Jun 29, 2023 •
edited
Loading

Closes: #38567

@thiagowfx thiagowfx changed the title script.callFunction: add missing serialization tests: iterator, generator, proxy script: add missing serialization tests: iterator, generator, proxy Jun 29, 2023
@thiagowfx thiagowfx enabled auto-merge (squash) June 29, 2023 14:40
@thiagowfx thiagowfx mentioned this pull request Jul 3, 2023
Closed
@thiagowfx
Copy link
Member Author

cc @whimboo @OrKoN

@whimboo
Copy link
Contributor

whimboo commented Jul 3, 2023

Could we please split adding new code and reformatting a file into different commits or PRs next time? It makes it really hard to review with such a lot of formatting changes.

whimboo
whimboo requested changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Actually please revert the formatting changes for this PR and only keep the addition of the new tests. I'll review once that has been done.

@thiagowfx
Copy link
Member Author

I made a comment when I sent the PR for review in the place where changes were done to make it easier to review.

That said I'll revert the formatting changes this time, but ideally we should introduce a formatter in this repo for the sake of reducing bikeshedding and noise.
Unfortunately in this case it was an unintended artifact from my text editor which is configured to run yapf in python files. It's a bit of a pain to disable it back-and-forth when switching projects.

@thiagowfx
Copy link
Member Author

Done.

@thiagowfx thiagowfx requested a review from whimboo July 3, 2023 11:24
@thiagowfx thiagowfx mentioned this pull request Jul 3, 2023
Merged
@thiagowfx thiagowfx disabled auto-merge July 3, 2023 12:59
@thiagowfx thiagowfx requested a review from whimboo July 3, 2023 13:35
@thiagowfx thiagowfx enabled auto-merge (squash) July 3, 2023 13:35
@thiagowfx thiagowfx disabled auto-merge July 3, 2023 13:36
@thiagowfx thiagowfx enabled auto-merge (squash) July 3, 2023 13:36
@whimboo
Copy link
Contributor

whimboo commented Jul 3, 2023

@jgraham would you mind to admin merge this PR? Thanks!

@thiagowfx thiagowfx mentioned this pull request Jul 3, 2023
Closed
@thiagowfx
Copy link
Member Author

cc @gsnedders for admin merge (as per #40860 (comment)).

@past
Copy link
Member

past commented Jul 5, 2023

I will admin merge this, but the slow tests need to be broken up to avoid having to admin merge changes to these files.

@jgraham jgraham disabled auto-merge July 5, 2023 11:05
@jgraham
Copy link
Contributor

jgraham commented Jul 5, 2023

Yeah, I think the Chrome failure is actually pretty legitimate; the tests are taking enough time that they're at high risk of causing stability issues (the Firefox failure seems to be different; for some reason we're only seeing a single set of results).

@jgraham jgraham merged commit 0374f91 into master Jul 5, 2023
14 of 17 checks passed
@jgraham jgraham deleted the thiagowfx/call_function branch July 5, 2023 11:07
@whimboo
Copy link
Contributor

whimboo commented Jul 5, 2023

Yeah, I think the Chrome failure is actually pretty legitimate; the tests are taking enough time that they're at high risk of causing stability issues (the Firefox failure seems to be different; for some reason we're only seeing a single set of results).

We do not have support for any of those types in Firefox yet. So it's expected to fail.

@thiagowfx could you please check how we can easily split this test file?

@thiagowfx
Copy link
Member Author

thiagowfx commented Jul 5, 2023 •
edited
Loading

Looking into splitting here: #40887

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

@whimboo whimboo whimboo approved these changes

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

@jgraham jgraham Awaiting requested review from jgraham

@jrandolf-2 jrandolf-2 Awaiting requested review from jrandolf-2

@juliandescottes juliandescottes Awaiting requested review from juliandescottes

@sadym-chromium sadym-chromium Awaiting requested review from sadym-chromium

@shs96c shs96c Awaiting requested review from shs96c

Assignees

@juliandescottes juliandescottes

Labels
webdriver-bidi
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing tests for WebDriver Bidi result Proxy
8 participants
@thiagowfx @whimboo @past @jgraham @OrKoN @juliandescottes @wpt-pr-bot @moz-wptsync-bot