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

Create a custom VM service extension and use it to implement the screenshot taking feature #593

Merged
merged 21 commits into from
Nov 15, 2022

Conversation

bartekpacia
Copy link
Contributor

Idea was sparked by @jakubfijalkowski in this comment.

@github-actions github-actions bot added the package: patrol Related to the patrol package (native automation, test bundling) label Nov 9, 2022
@bartekpacia bartekpacia changed the title Research of creation custom service extension for CLI <--> Dart test communication Research of creation custom service extension for CLI <--> app test communication Nov 9, 2022
@bartekpacia
Copy link
Contributor Author

Currently, I'm registering the service extension in the test and calling from the driver, but I want it the reverse.

Will poke around with this tomorrow.

* register service extension in the driver

* (wip) call driver's service extension from the test
@bartekpacia bartekpacia force-pushed the research/custom_service_extension branch from 6a9d8c5 to 06044c0 Compare November 10, 2022 11:02
@bartekpacia bartekpacia force-pushed the research/custom_service_extension branch from 88b260c to 5087214 Compare November 10, 2022 17:47
@bartekpacia
Copy link
Contributor Author

bartekpacia commented Nov 10, 2022

Works. Wow.

Caveats:

@bartekpacia bartekpacia changed the title Research of creation custom service extension for CLI <--> app test communication Add custom VM service extension and use it to implement the screenshot taking feature Nov 10, 2022
@bartekpacia bartekpacia changed the title Add custom VM service extension and use it to implement the screenshot taking feature Create a custom VM service extension and use it to implement the screenshot taking feature Nov 10, 2022
@bartekpacia bartekpacia marked this pull request as ready for review November 10, 2022 20:35
/// Shorthand for [nativeAutomator]. Throws if [nativeAutomator] is null.
NativeAutomator get native {
assert(nativeAutomator != null, 'native automator is null');
return nativeAutomator!;
}

HostAutomator get host {
assert(hostAutomator != null, 'host automator is null');
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually assert messages say what was expected, explain the needed invariants (for instance 'host getter can be used only if hostAutomator was initialized before' or something). Because currently the error message is not helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end, I improved the doc comment above native and host fields instead of improving the assertion message.

Albert told me some time ago that assertions should be in the form of:

X is null, Y is not in the inclusive 1..10 range

instead of:

X must not be null, Y must be in the inclusive 1..10 range

Copy link
Contributor

Choose a reason for hiding this comment

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

Albert told me some time ago that assertions should be in the form of:

X is null, Y is not in the inclusive 1..10 range

instead of:

X must not be null, Y must be in the inclusive 1..10 range

I cannot say I agree... But even then, I don't think such error messages are helpful at all. They don't give any meaningful information. I know what the value should or should not be, I can see what you are asserting (dart's assert is not a function, it is a language feature, they capture the expression and print the exact expression that was used to perform the assert, see attached screenshot). Asserts are for enforcing invariants so the error message should explain what kind of invariant was not fulfilled. Moving this to docs is great, though.

image

Coming back to this specific case, as an outside person who read the docs above this getter I still have no idea what should I do if I encounter a null here. Why would it be not initialized? Is it allowed to be not initialized? It is a final value, so I don't think I can initialize it myself.

@github-actions github-actions bot added the package: patrol_cli Related to the patrol_cli package label Nov 14, 2022
@bartekpacia bartekpacia force-pushed the research/custom_service_extension branch from d5005c1 to c493cde Compare November 15, 2022 13:59
@bartekpacia
Copy link
Contributor Author

I really want do write tests for this, but it will happen in another PR.

@bartekpacia bartekpacia force-pushed the research/custom_service_extension branch from e2a5e41 to 7d330a7 Compare November 15, 2022 15:30
@bartekpacia bartekpacia merged commit 5f1e477 into master Nov 15, 2022
@bartekpacia bartekpacia deleted the research/custom_service_extension branch November 15, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: patrol_cli Related to the patrol_cli package package: patrol Related to the patrol package (native automation, test bundling)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants