Refactor cli tests - #2287
Conversation
4231fc5 to
abcd4bc
Compare
* extracted common operations to functions in index_test_base * extracted shared setup to beforeEach in some suites * made suite structure more consistent
abcd4bc to
2090bd4
Compare
| ...workflowSettings, | ||
| ...workflowSettingsOverrides | ||
| }); | ||
| fs.writeFileSync(workflowSettingsPath, dumpYaml(workflowSettingsNew)); |
There was a problem hiding this comment.
Direct YAML dumping of a protobufjs instance: dumpYaml(workflowSettingsNew) dumps the class instance directly without calling dataform.WorkflowSettings.toObject(..., { enums: String }). This can serialize protobuf internal properties or dump enum values as numeric integers instead of string names.
| loadYaml(fs.readFileSync(workflowSettingsPath, "utf8")) | ||
| ); | ||
| const workflowSettingsNew = dataform.WorkflowSettings.create({ | ||
| ...workflowSettings, |
There was a problem hiding this comment.
Object spread on proto instance: Spreading ...workflowSettings into dataform.WorkflowSettings.create(...) copies instance-level own properties rather than pure schema values. Spreading the plain object loaded from YAML is cleaner.
| fs.writeFileSync(fullPath, content); | ||
| } | ||
|
|
||
| export async function alterWorkflowSettings(projectDir: string, workflowSettingsOverrides: Partial<dataform.IWorkflowSettings>): Promise<void> { |
There was a problem hiding this comment.
All file system calls are synchronous; no await is used
| fs.writeFileSync(fullPath, content); | ||
| } | ||
|
|
||
| export async function alterWorkflowSettings(projectDir: string, workflowSettingsOverrides: Partial<dataform.IWorkflowSettings>): Promise<void> { |
There was a problem hiding this comment.
Suggested fix accommodating mentioned issues:
export function alterWorkflowSettings(
projectDir: string,
workflowSettingsOverrides: Partial<dataform.IWorkflowSettings>
): void {
const workflowSettingsPath = path.join(projectDir, "workflow_settings.yaml");
const existingSettings = loadYaml(fs.readFileSync(workflowSettingsPath, "utf8")) as dataform.IWorkflowSettings;
const workflowSettings = dataform.WorkflowSettings.create({
...existingSettings,
...workflowSettingsOverrides
});
fs.writeFileSync(
workflowSettingsPath,
dumpYaml(dataform.WorkflowSettings.toObject(workflowSettings, { enums: String }))
);
}
This also aligns with what is currently proposed in setupProject.
|
|
||
| test("shows help for 'init' command", async () => { | ||
| const result = await getProcessResult(execFile(nodePath, [cliEntryPointPath, "help", "init"])); | ||
| const result = await runCli("help", "init"); |
There was a problem hiding this comment.
Here "init" is passed as dir, which is second parameter of runCli.
Either pass subcommands via options or provide an overload. I am not a fan of argsOrDir options, so lets figure out a way to solve this cleanly.
| stderr: string; | ||
| }> { | ||
| const args = [cliEntryPointPath, cmd, ...(dir !== undefined ? [dir] : []), ...options]; | ||
| return await getProcessResult( |
There was a problem hiding this comment.
I think you can drop await here, and return getProcessResult(...) will be sufficient.
| ]) | ||
| ); | ||
| beforeEach("setup test project", async () => { | ||
| projectDir = tmpDirFixture.createNewTmpDir() |
|
|
||
| test("with --disable-assertions flag", async () => { | ||
| await setUpWorkflowSettings(false); | ||
| await alterWorkflowSettings(projectDir, { disableAssertions : false }); |
There was a problem hiding this comment.
Linter should have fixed that, but lets be consistent and not have spaces before colons.
|
|
||
| test("with --job-labels flag", async () => { | ||
| await setUpWorkflowSettings(false); | ||
| await alterWorkflowSettings(projectDir, { disableAssertions : false }); |
There was a problem hiding this comment.
Also no space before colon.
2 participants
Note: this PR does not cover splitting larger test files into several seperate ones, this is planned but will be done in a seperate PR.