-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New MCP tool for running mobile tests (via app distribution). #9250
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
New MCP tool for running mobile tests (via app distribution). #9250
Conversation
… be used by an MCP tool.
Summary of ChangesHello @jrothfeder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Mobile Cloud Platform (MCP) tool to streamline the process of running mobile tests via app distribution. It achieves this by refactoring and centralizing the core app distribution and testing logic into a reusable function. This new function is now utilized by both the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this 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 introduces a new MCP tool for running mobile tests and refactors the app distribution logic for better reusability. The extraction of business logic from the CLI command into a shared distribute
function is a great improvement for maintainability.
I've found a couple of issues. There's a critical issue regarding a potential runtime error due to an unsafe non-null assertion. I've also pointed out some redundant code in the new MCP tool.
Overall, this is a solid contribution that improves the codebase structure. Addressing the feedback will make it even more robust.
'model=<model-id>,version=<os-version-id>,locale=<locale>,orientation=<orientation>'. Run 'gcloud firebase test android|ios models list' to see | ||
available devices.`, | ||
), | ||
testCaseIds: z.string().describe(`A comma-separated list of test case IDs.`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since running existing test cases is out of scope for this effort (right?), I figured instead of using test case IDs you would add the ability to run a test by calling CreateReleaseTest with the AiInstructions set: http://google3/google/firebase/appdistro/v1alpha/firebase_app_distribution.proto?l=783-785
That way you would be running a one-off test without have to call CreateTestCase first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's technically out of scope, however, it is far simpler to do so I figured we'd support both.
I was thinking we'd get this functionality in and then add the ability for one-off.
testDevices: z.string().describe( | ||
`Semicolon-separated list of devices to run automated tests on, in the format | ||
'model=<model-id>,version=<os-version-id>,locale=<locale>,orientation=<orientation>'. Run 'gcloud firebase test android|ios models list' to see | ||
available devices.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest also including some suggested device configurations, so Gemini can easily run a test without having to have the gcloud CLI installed. Something like:
"model=tokay,version=36,locale=en,orientation=portrait;model=e1q,version=34,locale=en,orientation=portrait" (Pixel 9 and Galaxy S24)
Side note: we could consider adding a tool to get the catalog, so we wouldn't have to rely on the gcloud CLI. Here's the API: https://firebase.google.com/docs/test-lab/reference/testing/rest/v1/testEnvironmentCatalog/get?apix_params=%7B%22environmentType%22%3A%22ANDROID%22%7D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this as the default.
Getting the catalog is a good idea. Maybe we'll flesh this out a bit more when we wire this up in to the prompt (that @tagboola is working on)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add it as a TODO, don't know if we'll add it in the short term though.
} | ||
|
||
async updateReleaseNotes(releaseName: string, releaseNotes: string): Promise<void> { | ||
async updateReleaseNotes(releaseName: string, releaseNotes?: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you ever call updateReleaseNotes
with releaseNotes
set to null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but this is how the code already worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code around parameter processing needs to be refactored to simplify all of this. All I did was move it to a shared-library. Would you be comfortable with addressing the parameter processing in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leave this the same i.e. don't make releaseNotes
nullable and then in the code where you call it, only call it if releaseNotes
is defined. I feel like that isn't too much of a change and can easily be addressed here.
* Value takes precedent over file. | ||
*/ | ||
export function parseIntoStringArray(value: string, file: string): string[] { | ||
export function parseIntoStringArray(value: string, file = ""): string[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default to null or undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is null or undefined preferable to the empty string in TS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I guess my Kotlin mind just assumed null was prefered, but maybe empty string is in TS.
* Value takes precedent over file. | ||
*/ | ||
export function parseTestDevices(value: string, file: string): TestDevice[] { | ||
export function parseTestDevices(value: string, file = ""): TestDevice[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, again I think we should revisit all of this. The existing code is a bit overcomplicated with how it handles parameters.
'model=<model-id>,version=<os-version-id>,locale=<locale>,orientation=<orientation>'. Run 'gcloud firebase test android|ios models list' to see | ||
available devices.`, | ||
), | ||
testCaseIds: z.string().describe(`A comma-separated list of test case IDs.`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're running on-off tests, I don't think we want to require testCaseIds since that would mean we are supporting running existing tests. If you're just keeping this here for now for simplicity and plan to update it in a future change I'm good with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking we could support both on-off and existing and this would be a good start. If you're comfortable, I'd like to see if we can get this PR in and then build the on-off functionality separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it makes sense to give Gemini access to a tool that requires test case IDs unless we also give it the ability to at least list test cases, and preferably to create test cases as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
inputSchema: z.object({ | ||
appId: ApplicationIdSchema, | ||
releaseBinaryFile: z.string().describe("Path to the binary release (APK)."), | ||
testDevices: z.string().describe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this optional and provide a default? I don't think we want to require the user to specify a device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll update the tool to send the following by default: model=MediumPhone.arm,version=34,locale=en,orientation=portrait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with zod you can set a default value
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
import type { ServerTool } from "../../tool"; | ||
import { run_tests } from "./tests"; | ||
|
||
export const appdistributionTools: ServerTool[] = [run_tests]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we place these behind the mcpalpha experiment?
} | ||
|
||
async updateReleaseNotes(releaseName: string, releaseNotes: string): Promise<void> { | ||
async updateReleaseNotes(releaseName: string, releaseNotes?: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leave this the same i.e. don't make releaseNotes
nullable and then in the code where you call it, only call it if releaseNotes
is defined. I feel like that isn't too much of a change and can easily be addressed here.
* Value takes precedent over file. | ||
*/ | ||
export function parseIntoStringArray(value: string, file: string): string[] { | ||
export function parseIntoStringArray(value: string, file = ""): string[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I guess my Kotlin mind just assumed null was prefered, but maybe empty string is in TS.
inputSchema: z.object({ | ||
appId: ApplicationIdSchema, | ||
releaseBinaryFile: z.string().describe("Path to the binary release (APK)."), | ||
testDevices: z.string().describe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with zod you can set a default value
testDevices: z.string().describe( | ||
`Semicolon-separated list of devices to run automated tests on, in the format | ||
'model=<model-id>,version=<os-version-id>,locale=<locale>,orientation=<orientation>'. Run 'gcloud firebase test android|ios models list' to see | ||
available devices.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add it as a TODO, don't know if we'll add it in the short term though.
18c29f8
into
feature-branch/mcp/mobile-testing
Description
New MCP tool for running mobile tests. Extracted business logic from appdistribution CLI so that it can be used by both that CLI and this tool.
Scenarios Tested
Ran the equivalent of
via Gemini cli. This can be initiated by typing
appdistribution
in the prompt, gemini cli will then walk you through providing the data needed to upload a new distribution binary and run tests.Sample Commands
initiated by typing
appdistribution
in the prompt, gemini cli will then walk you through providing the data needed to upload a new distribution binary and run tests.