throw error when filter in --only target is invalid#10577
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation for target filters in checkValidTargetFilters.ts to ensure that each target prefix is a valid deploy target, and adds a corresponding unit test. The review feedback highlights two important improvements: first, handling empty or whitespace-only targets in the validation loop to prevent confusing error messages, and second, avoiding the mutation of the shared SAMPLE_OPTIONS object in tests by using the object spread operator instead of Object.assign to prevent test pollution.
| const options = Object.assign(SAMPLE_OPTIONS, { | ||
| only: "functions:func1,func2", | ||
| }); |
There was a problem hiding this comment.
Using Object.assign(SAMPLE_OPTIONS, ...) mutates the shared SAMPLE_OPTIONS object, which can lead to test pollution and flaky tests if other tests rely on the default values of SAMPLE_OPTIONS.
Instead, use the object spread operator to create a shallow copy of SAMPLE_OPTIONS with the overridden properties.
const options = {
...SAMPLE_OPTIONS,
only: "functions:func1,func2",
};
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
LGTM once we add that spread operator
3 participants
Description
Fixes #6322. #6322 has 2 issues
func2is not a valid target since the expected format isfunctions:func2(product:target)Scenarios Tested
also verified that
firebase deploy --only functions:helloWorld,functions:helloWorld2deploys correctlylogs
Sample Commands
Notes
Is throwing an error too much? If so, we could instead opt for showing a warning message.