Skip to content

Commit bda9a91

Browse files
Copilotdmichon-msft
Copilot
and
dmichon-msft
committed
Address final PR feedback: improve logging and test coverage
Co-authored-by: dmichon-msft <26827560+dmichon-msft@users.noreply.github.com>
1 parent df43c85 commit bda9a91

5 files changed

Lines changed: 212 additions & 38 deletions

File tree

‎libraries/rush-lib/src/logic/operations/IPCOperationRunner.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,14 +85,16 @@ export class IPCOperationRunner implements IOperationRunner {
8585
async (terminal: ITerminal, terminalProvider: ITerminalProvider): Promise<OperationStatus> => {
8686
let isConnected: boolean = false;
8787
if (!this._ipcProcess || typeof this._ipcProcess.exitCode === 'number') {
88-
// Run the operation
89-
terminal.writeLine('Invoking: ' + this._commandToRun);
90-
91-
// Log any ignored parameters in verbose mode
88+
// Log any ignored parameters
9289
if (this._ignoredParameterValues.length > 0) {
93-
terminal.writeVerboseLine(`Ignored parameters: ${this._ignoredParameterValues.join(' ')}`);
90+
terminal.writeLine(
91+
`These parameters were ignored for this operation by project-level configuration: ${this._ignoredParameterValues.join(' ')}`
92+
);
9493
}
9594

95+
// Run the operation
96+
terminal.writeLine('Invoking: ' + this._commandToRun);
97+
9698
const { rushConfiguration, projectFolder } = this._rushProject;
9799

98100
const { environment: initialEnvironment } = context;

‎libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,16 @@ export class ShellOperationRunner implements IOperationRunner {
7676
async (terminal: ITerminal, terminalProvider: ITerminalProvider) => {
7777
let hasWarningOrError: boolean = false;
7878

79-
// Run the operation
80-
terminal.writeLine(`Invoking: ${this.commandToRun}`);
81-
82-
// Log any ignored parameters in verbose mode
79+
// Log any ignored parameters
8380
if (this._ignoredParameterValues.length > 0) {
84-
terminal.writeVerboseLine(`Ignored parameters: ${this._ignoredParameterValues.join(' ')}`);
81+
terminal.writeLine(
82+
`These parameters were ignored for this operation by project-level configuration: ${this._ignoredParameterValues.join(' ')}`
83+
);
8584
}
8685

86+
// Run the operation
87+
terminal.writeLine(`Invoking: ${this.commandToRun}`);
88+
8789
const { rushConfiguration, projectFolder } = this._rushProject;
8890

8991
const { environment: initialEnvironment } = context;

‎libraries/rush-lib/src/logic/operations/ShellOperationRunnerPlugin.ts‎

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -138,33 +138,29 @@ export interface ICustomParameterValuesForOperation {
138138
/**
139139
* Helper function to collect all parameter arguments for a phase
140140
*/
141-
function collectPhaseParameterArguments(phase: IPhase): Set<string> {
142-
const customParameterSet: Set<string> = new Set();
141+
function collectPhaseParameterArguments(phase: IPhase): string[] {
142+
const customParameterList: string[] = [];
143143
for (const tsCommandLineParameter of phase.associatedParameters) {
144-
const tempArgs: string[] = [];
145-
tsCommandLineParameter.appendToArgList(tempArgs);
146-
for (const arg of tempArgs) {
147-
customParameterSet.add(arg);
148-
}
144+
tsCommandLineParameter.appendToArgList(customParameterList);
149145
}
150-
return customParameterSet;
146+
return customParameterList;
151147
}
152148

153149
/**
154150
* Memoizer for custom parameter values by phase
155151
* @returns A function that returns the custom parameter values for a given phase
156152
*/
157153
export function getCustomParameterValuesByPhase(): (phase: IPhase) => ReadonlyArray<string> {
158-
const customParametersByPhase: Map<IPhase, Set<string>> = new Map();
154+
const customParametersByPhase: Map<IPhase, string[]> = new Map();
159155

160156
function getCustomParameterValuesForPhase(phase: IPhase): ReadonlyArray<string> {
161-
let customParameterSet: Set<string> | undefined = customParametersByPhase.get(phase);
162-
if (!customParameterSet) {
163-
customParameterSet = collectPhaseParameterArguments(phase);
164-
customParametersByPhase.set(phase, customParameterSet);
157+
let customParameterList: string[] | undefined = customParametersByPhase.get(phase);
158+
if (!customParameterList) {
159+
customParameterList = collectPhaseParameterArguments(phase);
160+
customParametersByPhase.set(phase, customParameterList);
165161
}
166162

167-
return Array.from(customParameterSet);
163+
return customParameterList;
168164
}
169165

170166
return getCustomParameterValuesForPhase;
@@ -178,29 +174,29 @@ export function getCustomParameterValuesByPhase(): (phase: IPhase) => ReadonlyAr
178174
export function getCustomParameterValuesByOperation(): (
179175
operation: Operation
180176
) => ICustomParameterValuesForOperation {
181-
const customParametersByPhase: Map<IPhase, Set<string>> = new Map();
177+
const customParametersByPhase: Map<IPhase, string[]> = new Map();
182178

183179
function getCustomParameterValuesForOp(operation: Operation): ICustomParameterValuesForOperation {
184180
const { associatedPhase: phase, settings } = operation;
185181

186182
// Check if there are any parameters to ignore
187183
const parameterNamesToIgnore: string[] | undefined = settings?.parameterNamesToIgnore;
188184
if (!parameterNamesToIgnore || parameterNamesToIgnore.length === 0) {
189-
// No filtering needed - use the cached parameter set for efficiency
190-
let customParameterSet: Set<string> | undefined = customParametersByPhase.get(phase);
191-
if (!customParameterSet) {
192-
customParameterSet = collectPhaseParameterArguments(phase);
193-
customParametersByPhase.set(phase, customParameterSet);
185+
// No filtering needed - use the cached parameter list for efficiency
186+
let customParameterList: string[] | undefined = customParametersByPhase.get(phase);
187+
if (!customParameterList) {
188+
customParameterList = collectPhaseParameterArguments(phase);
189+
customParametersByPhase.set(phase, customParameterList);
194190
}
195191

196192
return {
197-
parameterValues: Array.from(customParameterSet),
193+
parameterValues: customParameterList,
198194
ignoredParameterValues: []
199195
};
200196
}
201197

202198
// Filtering is needed - we must iterate through parameter objects to check longName
203-
// Note: We cannot use the cached parameter set here because we need access to
199+
// Note: We cannot use the cached parameter list here because we need access to
204200
// the parameter objects to get their longName property for filtering
205201
const ignoreSet: Set<string> = new Set(parameterNamesToIgnore);
206202
const filteredParameterValues: string[] = [];

‎libraries/rush-lib/src/logic/operations/test/ShellOperationRunnerPlugin.test.ts‎

Lines changed: 178 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,31 @@
44
import path from 'node:path';
55
import { JsonFile } from '@rushstack/node-core-library';
66
import { ConsoleTerminalProvider, Terminal } from '@rushstack/terminal';
7+
import {
8+
CommandLineAction,
9+
type CommandLineParameter,
10+
type CommandLineFlagParameter,
11+
type CommandLineStringParameter,
12+
type CommandLineChoiceParameter,
13+
type CommandLineStringListParameter
14+
} from '@rushstack/ts-command-line';
715

816
import { RushConfiguration } from '../../../api/RushConfiguration';
9-
import { CommandLineConfiguration, type IPhasedCommandConfig } from '../../../api/CommandLineConfiguration';
17+
import {
18+
CommandLineConfiguration,
19+
type IPhasedCommandConfig,
20+
type IParameterJson
21+
} from '../../../api/CommandLineConfiguration';
1022
import type { Operation } from '../Operation';
11-
import type { ICommandLineJson } from '../../../api/CommandLineJson';
23+
import type { ICommandLineJson, ParameterJson } from '../../../api/CommandLineJson';
1224
import { PhasedOperationPlugin } from '../PhasedOperationPlugin';
1325
import { ShellOperationRunnerPlugin } from '../ShellOperationRunnerPlugin';
1426
import {
1527
type ICreateOperationsContext,
1628
PhasedCommandHooks
1729
} from '../../../pluginFramework/PhasedCommandHooks';
1830
import { RushProjectConfiguration } from '../../../api/RushProjectConfiguration';
31+
import { RushConstants } from '../../RushConstants';
1932

2033
interface ISerializedOperation {
2134
name: string;
@@ -29,6 +42,106 @@ function serializeOperation(operation: Operation): ISerializedOperation {
2942
};
3043
}
3144

45+
/**
46+
* Helper function to create CommandLineParameter instances from parameter definitions.
47+
* Extracted logic from BaseScriptAction.defineScriptParameters()
48+
*/
49+
function createCommandLineParameters(
50+
action: CommandLineAction,
51+
associatedParameters: Set<IParameterJson>
52+
): Map<string, CommandLineParameter> {
53+
const customParameters: Map<string, CommandLineParameter> = new Map();
54+
55+
for (const parameter of associatedParameters) {
56+
let tsCommandLineParameter: CommandLineParameter | undefined;
57+
58+
switch (parameter.parameterKind) {
59+
case 'flag':
60+
tsCommandLineParameter = action.defineFlagParameter({
61+
parameterShortName: parameter.shortName,
62+
parameterLongName: parameter.longName,
63+
description: parameter.description,
64+
required: parameter.required
65+
});
66+
break;
67+
case 'choice':
68+
tsCommandLineParameter = action.defineChoiceParameter({
69+
parameterShortName: parameter.shortName,
70+
parameterLongName: parameter.longName,
71+
description: parameter.description,
72+
required: parameter.required,
73+
alternatives: parameter.alternatives.map((x) => x.name),
74+
defaultValue: parameter.defaultValue
75+
});
76+
break;
77+
case 'string':
78+
tsCommandLineParameter = action.defineStringParameter({
79+
parameterLongName: parameter.longName,
80+
parameterShortName: parameter.shortName,
81+
description: parameter.description,
82+
required: parameter.required,
83+
argumentName: parameter.argumentName
84+
});
85+
break;
86+
case 'integer':
87+
tsCommandLineParameter = action.defineIntegerParameter({
88+
parameterLongName: parameter.longName,
89+
parameterShortName: parameter.shortName,
90+
description: parameter.description,
91+
required: parameter.required,
92+
argumentName: parameter.argumentName
93+
});
94+
break;
95+
case 'stringList':
96+
tsCommandLineParameter = action.defineStringListParameter({
97+
parameterLongName: parameter.longName,
98+
parameterShortName: parameter.shortName,
99+
description: parameter.description,
100+
required: parameter.required,
101+
argumentName: parameter.argumentName
102+
});
103+
break;
104+
case 'integerList':
105+
tsCommandLineParameter = action.defineIntegerListParameter({
106+
parameterLongName: parameter.longName,
107+
parameterShortName: parameter.shortName,
108+
description: parameter.description,
109+
required: parameter.required,
110+
argumentName: parameter.argumentName
111+
});
112+
break;
113+
case 'choiceList':
114+
tsCommandLineParameter = action.defineChoiceListParameter({
115+
parameterShortName: parameter.shortName,
116+
parameterLongName: parameter.longName,
117+
description: parameter.description,
118+
required: parameter.required,
119+
alternatives: parameter.alternatives.map((x) => x.name)
120+
});
121+
break;
122+
default:
123+
throw new Error(
124+
`${RushConstants.commandLineFilename} defines a parameter "${
125+
(parameter as ParameterJson).longName
126+
}" using an unsupported parameter kind "${(parameter as ParameterJson).parameterKind}"`
127+
);
128+
}
129+
130+
customParameters.set(parameter.longName, tsCommandLineParameter);
131+
}
132+
133+
return customParameters;
134+
}
135+
136+
/**
137+
* Test implementation of CommandLineAction for testing parameter handling
138+
*/
139+
class TestCommandLineAction extends CommandLineAction {
140+
protected async onExecuteAsync(): Promise<void> {
141+
// No-op for testing
142+
}
143+
}
144+
32145
describe(ShellOperationRunnerPlugin.name, () => {
33146
it('shellCommand "echo custom shellCommand" should be set to commandToRun', async () => {
34147
const rushJsonFile: string = path.resolve(__dirname, `../../test/customShellCommandinBulkRepo/rush.json`);
@@ -148,6 +261,58 @@ describe(ShellOperationRunnerPlugin.name, () => {
148261
terminal
149262
);
150263

264+
// Create a dummy CommandLineAction to host the parameters
265+
const action: TestCommandLineAction = new TestCommandLineAction({
266+
actionName: 'build',
267+
summary: 'Test build action',
268+
documentation: 'Test'
269+
});
270+
271+
// Create CommandLineParameter instances from the parameter definitions
272+
const customParametersMap: Map<string, CommandLineParameter> = createCommandLineParameters(
273+
action,
274+
buildCommand.associatedParameters
275+
);
276+
277+
// Set values on the parameters to test filtering
278+
// Set --production flag
279+
const productionParam = customParametersMap.get('--production') as CommandLineFlagParameter | undefined;
280+
if (productionParam) {
281+
(productionParam as unknown as { _setValue(value: boolean): void })._setValue(true);
282+
}
283+
284+
// Set --verbose flag
285+
const verboseParam = customParametersMap.get('--verbose') as CommandLineFlagParameter | undefined;
286+
if (verboseParam) {
287+
(verboseParam as unknown as { _setValue(value: boolean): void })._setValue(true);
288+
}
289+
290+
// Set --config parameter
291+
const configParam = customParametersMap.get('--config') as CommandLineStringParameter | undefined;
292+
if (configParam) {
293+
(configParam as unknown as { _setValue(value: string): void })._setValue('/path/to/config.json');
294+
}
295+
296+
// Set --mode parameter
297+
const modeParam = customParametersMap.get('--mode') as CommandLineChoiceParameter | undefined;
298+
if (modeParam) {
299+
(modeParam as unknown as { _setValue(value: string): void })._setValue('prod');
300+
}
301+
302+
// Set --tags parameter
303+
const tagsParam = customParametersMap.get('--tags') as CommandLineStringListParameter | undefined;
304+
if (tagsParam) {
305+
(tagsParam as unknown as { _setValue(value: string[]): void })._setValue(['tag1', 'tag2']);
306+
}
307+
308+
// Update the phase's associatedParameters to use our created CommandLineParameters
309+
for (const phase of buildCommand.phases) {
310+
phase.associatedParameters.clear();
311+
for (const param of customParametersMap.values()) {
312+
phase.associatedParameters.add(param);
313+
}
314+
}
315+
151316
const fakeCreateOperationsContext: Pick<
152317
ICreateOperationsContext,
153318
| 'phaseOriginal'
@@ -164,7 +329,7 @@ describe(ShellOperationRunnerPlugin.name, () => {
164329
projectsInUnknownState: new Set(rushConfiguration.projects),
165330
projectConfigurations,
166331
rushConfiguration,
167-
customParameters: new Map()
332+
customParameters: customParametersMap
168333
};
169334

170335
const hooks: PhasedCommandHooks = new PhasedCommandHooks();
@@ -185,12 +350,21 @@ describe(ShellOperationRunnerPlugin.name, () => {
185350
const commandHashA = operationA!.runner!.getConfigHash();
186351
// Should not contain --production but should contain other parameters
187352
expect(commandHashA).not.toContain('--production');
353+
expect(commandHashA).toContain('--verbose');
354+
expect(commandHashA).toContain('--config');
355+
expect(commandHashA).toContain('--mode');
356+
expect(commandHashA).toContain('--tags');
188357

189358
// Verify that project 'b' has all parameters (no filtering)
190359
const operationB = Array.from(operations).find((op) => op.name === 'b');
191360
expect(operationB).toBeDefined();
361+
const commandHashB = operationB!.runner!.getConfigHash();
192362
// Should contain all parameters since no filtering is configured
193-
// Note: Parameters only appear if they are provided, so we can't test for them without setting them
363+
expect(commandHashB).toContain('--production');
364+
expect(commandHashB).toContain('--verbose');
365+
expect(commandHashB).toContain('--config');
366+
expect(commandHashB).toContain('--mode');
367+
expect(commandHashB).toContain('--tags');
194368

195369
// All projects snapshot
196370
expect(Array.from(operations, serializeOperation)).toMatchSnapshot();

‎libraries/rush-lib/src/logic/operations/test/__snapshots__/ShellOperationRunnerPlugin.test.ts.snap‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
exports[`ShellOperationRunnerPlugin parameters should be filtered when parameterNamesToIgnore is specified 1`] = `
44
Array [
55
Object {
6-
"commandToRun": "echo building a ",
6+
"commandToRun": "echo building a --verbose --config /path/to/config.json --mode prod --tags tag1 --tags tag2",
77
"name": "a",
88
},
99
Object {
10-
"commandToRun": "echo building b ",
10+
"commandToRun": "echo building b --production --verbose --config /path/to/config.json --mode prod --tags tag1 --tags tag2",
1111
"name": "b",
1212
},
1313
]

0 commit comments

Comments
 (0)