-
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
Changes from 6 commits
b9d24b1
46f06fd
8b028c2
844b3a2
6b0b3de
1771da9
3392c6c
81cda3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ import { FieldHints, LoginCredential, TestDevice } from "./types"; | |
* file and converts the input into an string[]. | ||
* 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 commentThe 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 commentThe 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 commentThe 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. |
||
// If there is no value then the file gets parsed into a string to be split | ||
if (!value && file) { | ||
ensureFileExists(file); | ||
|
@@ -61,7 +61,10 @@ export function getAppName(options: any): string { | |
if (!options.app) { | ||
throw new FirebaseError("set the --app option to a valid Firebase app id and try again"); | ||
} | ||
const appId = options.app; | ||
return toAppName(options.app); | ||
} | ||
|
||
export function toAppName(appId: string) { | ||
return `projects/${appId.split(":")[1]}/apps/${appId}`; | ||
} | ||
|
||
|
@@ -70,7 +73,7 @@ export function getAppName(options: any): string { | |
* and converts the input into a string[] of test device strings. | ||
* 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 commentThe 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 commentThe 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. |
||
// If there is no value then the file gets parsed into a string to be split | ||
if (!value && file) { | ||
ensureFileExists(file); | ||
|
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
withreleaseNotes
set tonull
?Uh oh!
There was an error while loading. Please reload this page.
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 ifreleaseNotes
is defined. I feel like that isn't too much of a change and can easily be addressed here.