Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Jul 3, 2018

Forked from https://github.com/flutter/flutter/pull/19005/files

  • Adds a new flutter driver command to get the semantic node id for the specified finder or it's ancestor.
  • This is required for semantics integration tests in which we will use platform channels to match the native semantics data based on node id

@jonahwilliams jonahwilliams changed the title Flutter driver updates Add getSemanticsId command to flutter_driver Jul 3, 2018
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Tests?

return new ByType(json['type']);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove one empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}


/// A Flutter driver command that retrieves a semantics id using a specified finder.
Copy link
Member

Choose a reason for hiding this comment

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

mention here as well that it might be the semantics information from an ancestor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams
Copy link
Contributor Author

Tests added

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM with minor nits.

/// then the semantics node of the first ancestor is returned instead.
///
/// If the finder returns multiple objects, or there are no semantics nodes then
/// a [StateError] is thrown by the extension.
Copy link
Member

Choose a reason for hiding this comment

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

Do we define anywhere what "the extension" is? Or is it just an implementation detail that the error is thrown by the extension and we should maybe omit that detail from documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error thrown on the app side becomes an error thrown in the driver script, it isn't necessary to mention that is thrown on the device so i will omit this part

return result.changedState;
}

/// Retrieves the semantics node id for the object returned by [finder], or
Copy link
Member

Choose a reason for hiding this comment

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

I believe arguments mentioned in doc comments should be enclosed in `` instead of [] unless the argument is also a field on the class.

(here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// Semantics must be enabled to use this method, either using a platform
/// specific shell command or [setSemantics].
Future<int> getSemanticsId(SerializableFinder finder, { Duration timeout = _kShortTimeout}) async {
final GetSemanticsIdResult result = GetSemanticsIdResult.fromJson(await _sendCommand(new GetSemanticsId(finder, timeout: timeout)));
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe add some line breaks to this line to make it easier to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

RenderObject renderObject = element.renderObject;
SemanticsNode node;
while (renderObject != null && node == null) {
node = renderObject.debugSemantics;
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: It's guaranteed that drive tests always run with asserts enabled? Otherwise debugSemantics would be null even when semantics are enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not always (microbenchmarks), but for these integration tests we'll always be using a debug build. Added doc comment to the command.

@jonahwilliams jonahwilliams merged commit 7dd265f into flutter:master Jul 9, 2018
@jonahwilliams jonahwilliams deleted the flutter_driver_updates branch July 9, 2018 22:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants