Skip to content

Flutter driver deserialization #172927

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
Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Flutter driver deserialization #172927

wants to merge 13 commits into from
+511 −88

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Jul 29, 2025 •
edited
Loading

Adds more descriptive ArgumentErrors to flutter driver deserialization methods for commands and finders.

Also adds support for the normal defaults to the deserialization handlers (can revert this if you like, but it helps when interacting with flutter driver as a client so that it is more similar to how the API works).

The tests aren't completely exhaustive as that would get very verbose, but they do cover a variety of use cases.

Fixes #172127

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@jakemac53 jakemac53 requested a review from matanlurey as a code owner July 29, 2025 21:27
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Jul 29, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 29, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request significantly improves the deserialization logic in flutter_driver by adding more descriptive ArgumentErrors and support for default values, making the API more robust and easier to debug. The introduction of a path parameter for tracking the location of deserialization errors in nested structures is a particularly valuable enhancement. The accompanying tests are thorough and cover the new functionality well.

I've provided a couple of minor suggestions to further refine the new error handling and its corresponding tests. Overall, this is an excellent and well-executed contribution.

@jakemac53 jakemac53 force-pushed the flutter-driver-deserialization branch from d3d6665 to 7bd1ddc Compare July 29, 2025 21:30
matanlurey
matanlurey approved these changes Aug 11, 2025
View reviewed changes
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

LGTM sorry for the delay (OOO)

@chunhtai chunhtai removed the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Aug 13, 2025
@github-actions github-actions bot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Aug 18, 2025
@jakemac53 jakemac53 enabled auto-merge August 18, 2025 15:52
@jakemac53 jakemac53 disabled auto-merge August 18, 2025 16:43
@jakemac53
Copy link
Contributor Author

jakemac53 commented Aug 18, 2025 •
edited
Loading

cc @matanlurey PTAL at the small change I made, I removed the path argument from FinderExtension.deserialize - this wasn't necessary and was actually an accidental breaking change since that method is intended to be implemented by user implementations.

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

@matanlurey matanlurey matanlurey approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flutter driver should provide better error handling for malformed RPCs
3 participants
@jakemac53 @matanlurey @chunhtai