Repository navigation
Conversation
camsim99
left a comment
There was a problem hiding this comment.
First pass review
| CI does not run this package's integration tests on emulators, | ||
| so they must be run locally before pushing. |
There was a problem hiding this comment.
Is this true ? We want them to be run locally either way, yes, but please confirm this
|
|
||
| ```bash | ||
| dart run .agents/skills/run-on-emulator/tool/run.dart integration-test | ||
| ``` |
There was a problem hiding this comment.
do we need this command here if we are already referring them to skill.md?
There was a problem hiding this comment.
Should this just be named LICENSE to follow the pattern in this repo?
| agent will perform with `run-on-emulator` (`app` or `integration-test`). | ||
| Do NOT ask the user to run an emulator, launch the app, or run integration | ||
| tests by hand. | ||
| - **When executing a plan**, carry out its verification steps, including |
There was a problem hiding this comment.
Do we really need to say this explicitly if we tell the agent to proceed with the plan that has those steps in it already?
| - Only assign verification to the user when it needs physical hardware | ||
| (e.g., flash/torch, real lens characteristics, OEM quirks) or is a subtle | ||
| visual property that cannot be asserted in a test (e.g., mirroring), and | ||
| state why. |
There was a problem hiding this comment.
Can this be combined with the first bullet point in some way for clarity if it would help the agent and be more efficient? Something more like if it needs physical hardware, then xyz. if not, abc.
| | 7 | Integration tests failed or timed out | Report each failing test and its error from the output. Re-run each failing test once with `--plain-name` to tell flakes (common right after a cold boot) from real failures, and report both results. On a timeout, take a screenshot (see [Verifying a Change](#verifying-a-change)) to find what is blocking the app. | | ||
| | 64 | Invalid arguments | Fix the command and re-run. | | ||
|
|
||
| ## Writing Verification Steps in Implementation Plans |
There was a problem hiding this comment.
this seems out of scope for this skill. if anything, this should live in agents.md. we already have a similar not there that probably covers this but please confirm
|
|
||
| Leave out the user section entirely if nothing needs it. | ||
|
|
||
| ## Verifying a Change |
There was a problem hiding this comment.
this should be cut. all content should be covered in the "should u use an emulator for this?" section and agents.md to tell agents when to even look here (to loose detail because the ^^ section can answer that in more detail). TLDR: this feel redundant.
| driver tools for verification: they can only see widgets, not plugin state, | ||
| and require changes to the example app. Write an integration test instead. | ||
|
|
||
| ### Screenshots |
There was a problem hiding this comment.
actually not sure if we should include this. the agent can verify behavior on the emulator and report back without a screenshot. is the point here (and if so, maybe we can clarify) that if we are testing for a visual change, you can present a screenshot as part of the results to prove the result?
| the user exact manual steps; you may attach the screenshot as context, but do | ||
| not present it as proof. | ||
|
|
||
| ### Device Recipes |
There was a problem hiding this comment.
I guess this is okay but I thought we should prefer writing integration tests and running that versus adb stuff and therefore, at least for now, we can keep it light on the adb guidance and point to the skill (if it's helpful) to say oh if you need this, look there
There was a problem hiding this comment.
that preference for integration tests (if you agree) should be (if it is not already) part of agents.md
There was a problem hiding this comment.
I haven't reviewed all of this yet. I want you to review my comments on the related skill and make edits here first. I really want to cut this down as much as possible. We have an amazing plugins tool in the repo (I assume this calls out to it a good bit) and we should make use of preexisting behavior as much as possible versus re-inventing the wheel. Please keep that in mind as you refactor this down for my review.
| CI also runs these tests on a physical device (Firebase Test Lab), | ||
| but catching failures locally avoids a slow CI round trip. |
There was a problem hiding this comment.
Delete; I don't think the agent needs this context in this scope.
| | Preview rendering and switching cameras | Flash or torch output | | ||
| | Image capture, video recording, and image streaming | Real lens characteristics (optical zoom, focus distance, multiple back cameras, extensions) | | ||
| | Exposure, zoom, and focus/metering calls and their reported ranges | Exact resolution preset sizes (emulated cameras do not support every preset) | | ||
| | Rotation, lifecycle (background/resume), and permission flows | Device-specific sensor orientations, OEM quirks, or performance | |
There was a problem hiding this comment.
Is the table format helpful here? Examples on the same row seem unrelated, but I'm flexible.
There was a problem hiding this comment.
either way we should probably note that these are examples and not an exhaustive list
| 1. **Check the Android CLI:** run `android --version`. It must print version | ||
| 1.1 or newer. If the command is not found or prints no version, stop: the | ||
| [Android CLI](https://developer.android.com/tools/agents) is required. If | ||
| the version is older, stop and ask the user to run `android update`. | ||
| 2. **Reuse a running emulator:** run `adb devices` (if `adb` is not on the | ||
| PATH, use `"$(android info sdk)/platform-tools/adb"`). If it lists | ||
| `emulator-<port> device`, note that serial and skip to section 3 or 4. | ||
| 3. **Pick an AVD:** run `android emulator list`. If it lists none, stop and | ||
| suggest `android emulator create medium_phone`. Otherwise use the AVD the | ||
| user named, or the first one. | ||
| 4. **Check that the AVD has a camera:** run | ||
| `grep hw.camera ~/.android/avd/<avd>.avd/config.ini`. If `hw.camera.back` | ||
| and `hw.camera.front` are both `none`, stop: camera tests would pass without | ||
| testing anything. (A missing `hw.camera.back` means `emulated`.) | ||
| 5. **Start it:** run `android emulator start --headless <avd>` (omit | ||
| `--headless` if the user wants to watch). It returns once the emulator has | ||
| booted. If it fails, stop and suggest `android emulator start --cold <avd>`. | ||
| Run `adb devices` to get the serial. |
There was a problem hiding this comment.
Is any of this redundant wit the android-cli tool? This looks ok to me but just wondering if there is even more to cut down here.
|
|
||
| ## 3. Run the integration tests | ||
|
|
||
| 1. **Grant permissions first, before every run.** Otherwise the camera |
There was a problem hiding this comment.
clarify "run of...."
| Leave the emulator running for later runs. If you started it, stop it with | ||
| `android emulator stop <serial>` when you are done. | ||
|
|
||
| ## 3. Run the integration tests |
There was a problem hiding this comment.
It's odd that this is 3 and the next section is 4. most times, the agent only needs/wants to do 3 or 4. Can we somehow notify that here like maybe we have a 3a/3b vs 3/4
| 2. **Interact with it** using the [android-cli](../android-cli/SKILL.md) skill | ||
| (`android layout`, `android input`). The example app's camera controls have | ||
| no text or descriptions, so tap them by class, e.g. | ||
| `android input --device=<serial> tap clz=RadioButton --match=first` selects | ||
| the first camera. If a flow needs more than about five taps, write an | ||
| integration test instead. Do not add Flutter Driver to the example app. |
There was a problem hiding this comment.
Can you confirm how relevant this is for Flutter apps?
| that it is rotated by 90° or 180°, or that a control or error appeared. It | ||
| cannot show aspect ratio, mirroring, cropping, exposure, color, or frame | ||
| rate: assert those in a test, or report them as not verified. | ||
| 4. **For rotation bugs**, lock the orientation so results are deterministic, |
There was a problem hiding this comment.
Do we want this? A lot of the time, a bug repros without locking so we don't want to lock just in case that makes a difference. Maybe this can be a suggestion ?
WIP :)
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2