fix(testing): fix tests on windows #131

Tlacenka · 2023-10-18T13:01:07Z

In this PR, I identified and fixed various issue while running tests in the Windows environment:

shell needs to be specified to run programs in command shell on Windows, as described here
path notation on Windows is different
JSON formatting is different for UNIX and Windows
writing into files replaced by echo command which works seamlessly on Windows
glob patterns require extra quotation marks for argument expansion on Linux

I also extracted the runner config for writing report into a file to a separate function (there was a lot of duplicate code to keep consistent).

matejchalk

Really great to have this fixed finally, so that the codebase is now usable on Windows 😅

As discussed, I created a follow-up for automatically checking it: #141

matejchalk · 2023-10-24T10:21:46Z

packages/utils/src/lib/file-write-runner-config.spec.ts

+    const osAuditOutput =
+      platform() === 'win32'
+        ? JSON.stringify(audits)
+        : "'" + JSON.stringify(audits) + "'";
+
+    const filePath = join(process.cwd(), 'tmp/report.json');
+    const unixFilePath = toUnixPath(filePath);
+
+    expect(createFileWriteRunnerConfig(audits, filePath)).toStrictEqual({
+      command: 'echo',
+      args: [osAuditOutput, '>', unixFilePath],
+      outputFile: unixFilePath,
+    });


The test code is basically the same as the function it's testing. If we're gonna test this (not sure it makes sense to test a testing utility 🤔), I would have different test cases per OS (mock os.platform() value).

If I did that, I would then be testing the unix path function which already has separate tests. Removed.

matejchalk · 2023-10-24T10:23:19Z

packages/utils/src/lib/file-write-runner-config.ts

+    args: [auditOutput, '>', toUnixPath(outputFilePath)],
+    outputFile: toUnixPath(outputFilePath),


Should these be UNIX paths on Windows? 🤨

maybe path.join or path.normalize could be used instead

I think just outputFile: outputFilePath should be fine, the file path should be platform-dependant.

The toUnixPath function is needed by the ESLint plugin, because it emits source file issues and needs to convert them from absolute platform-dependant paths to relative Git paths. I'm not aware of any other use case.

If you say so. I wanted to make sure that the resulting path would be safe but if that is the responsibility of the caller, sure.

matejchalk · 2023-10-24T10:30:42Z

packages/cli/package.json

@@ -7,6 +7,7 @@
  "dependencies": {
    "@code-pushup/models": "*",
    "@code-pushup/core": "*",
+    "@code-pushup/utils": "*",


As far as I can tell, @code-pushup/utils is imported only in test files and packages/cli/code-pushup.config.ts. So I don't think this dependency should be here. The lint rule takes only imports in "production" files into account, which are configured here in nx.json.

I believe we're just missing a glob which will exclude the config files (since we're only using those for testing) - probably something like ['**/*/code-pushup.config.*.ts', '**/*/code-pushup.config.*.js', '**/*/code-pushup.config.*.mjs'].

Alternatively, we can rename this file to code-pushup.config.mock.ts and then it should be excluded without changing the configuration. Maybe that's better, because it's explicitly marked as testing-only 🤔

BioPhoton

Nice improvement of testing! Left some comments.

BioPhoton · 2023-10-24T21:14:49Z

e2e/cli-e2e/tests/collect.spec.ts

-        '--verbose',
-        '--persist.format=stdout',
-      ],
+      args: [cliPath, 'collect', '--verbose', '--persist.format=stdout'],


You could use
https://github.com/flowup/quality-metrics-cli/blob/090e34d64c6ccb8ced01a615ea071c2fda83ea97/packages/utils/src/lib/execute-process.ts#L189

to generate the args in a more maintainable way

This test was written by @matejchalk, I merely extracted one of the arguments to a variable. What do you think about this suggestion?

From my point of view, having arguments as an explicit array increases the test readability which is what you want in test code.

I also don't see the point of objectToCliArgs, to be honest. This CLI notation is what the user sees, which makes it more fitting for an E2E test, the wrapper function just obfuscates it.

BioPhoton · 2023-10-24T21:19:08Z

packages/cli/test/config.mock.cjs

-              },
-            ],
-          )}')`,
+      runner: createFileWriteRunnerConfig(


we have mock functions in models for that situation living in the test/fixture folder

This is a function that is reusable for multiple tests (currently in cli and lighthouse plugin) and otherwise would need to be duplicated. In one of my new issues, I plan to address this by extracting reusable functions to a separate package instead of duplicating logic in multiple packages.

BioPhoton · 2023-10-24T21:21:42Z

packages/plugin-lighthouse/src/lib/lighthouse-plugin.ts

 import { AuditOutputs, PluginConfig } from '@code-pushup/models';
+import { createFileWriteRunnerConfig } from '@code-pushup/utils';


Lets move this into models/test/fixtures/runner.config.mock.ts please

Unfortunately, this function cannot live in the models package as it would cause a circular dependency. Until a separate testing utils package is created, Matthew advised me to add it to utils and account for one separate usage by duplicating the function logic.

BioPhoton · 2023-10-24T21:23:50Z

tsconfig.base.json

@@ -17,6 +17,7 @@
    "skipDefaultLibCheck": true,
    "baseUrl": ".",
    "resolveJsonModule": true,
+    "allowSyntheticDefaultImports": true,


For what code do we need this?

In order to mock platform for unit tests, I needed to create a spy for the os.platform() which required import directly from os. Unfortunately, this caused a default export TS error and I had to override it with this option.

Tlacenka · 2023-10-27T10:46:38Z

There were too many conflicts so I created a new branch and PR #165

In this PR, I identified and fixed various issue while running tests in the Windows environment: - shell needs to be specified to run programs in command shell on Windows, as described [here](https://stackoverflow.com/questions/60386867/node-spawn-child-process-not-working-in-windows) - path notation on Windows is different - JSON formatting is different for UNIX and Windows - writing into files replaced by echo command which works seamlessly on Windows - glob patterns require extra quotation marks for argument expansion on Linux - recreating tmp folder caused issues for me so I partially fixed it via adding the recursive option Note: This is the same scope as #131. Unfortunately, merging changes from the test refactor did not go well so I started off a clean branch and re-applied the changes.

Tlacenka requested a review from matejchalk October 18, 2023 13:01

Tlacenka added 4 commits October 18, 2023 15:21

feat(utils): add function for UNIX path transformation

179fbd5

fix(testing): adapt tests to Windows paths

5a6aeea

fix(cli): adapt writing to file for Windows

2f88ed0

fix(utils): define shell program for windows subprocess spawn

f7f2298

Tlacenka force-pushed the fix-tests-on-windows branch from b6aa6f3 to f7f2298 Compare October 18, 2023 13:29

Tlacenka added 2 commits October 18, 2023 16:30

feat(utils): extract file write runner config to a function

f7c1310

fix(testing): fix javascript imports

902156f

BioPhoton added the 🔬 testing writing tests label Oct 20, 2023

fix(testing): adjust JSON format to OS

8f34d19

Tlacenka force-pushed the fix-tests-on-windows branch from 0a895df to 8f34d19 Compare October 23, 2023 11:48

fix(cli,plugin-lighthouse): include utils dependency

a51d2e2

Tlacenka changed the title ~~Fix tests on windows~~ fix(testing): fix tests on windows Oct 23, 2023

matejchalk mentioned this pull request Oct 24, 2023

Test on Windows in CI #141

Closed

1 task

Tlacenka force-pushed the fix-tests-on-windows branch 2 times, most recently from 941f27d to c6155df Compare October 24, 2023 09:51

fix(testing): handle expanded glob patterns

c9a589c

Tlacenka force-pushed the fix-tests-on-windows branch from c6155df to c9a589c Compare October 24, 2023 09:57

Tlacenka requested a review from BioPhoton October 24, 2023 10:01

Tlacenka added this to the 1. Internal MVP milestone Oct 24, 2023

Tlacenka self-assigned this Oct 24, 2023

matejchalk reviewed Oct 24, 2023

View reviewed changes

BioPhoton requested changes Oct 24, 2023

View reviewed changes

matejchalk mentioned this pull request Oct 26, 2023

refactor(utils): add progress visualisation logic #139

Merged

Tlacenka mentioned this pull request Oct 27, 2023

fix(testing): fix tests for Windows #165

Merged

Tlacenka closed this Oct 27, 2023

Tlacenka deleted the fix-tests-on-windows branch October 27, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(testing): fix tests on windows #131

fix(testing): fix tests on windows #131

Tlacenka commented Oct 18, 2023 •

edited

Loading

matejchalk left a comment

matejchalk Oct 24, 2023

Tlacenka Oct 25, 2023

matejchalk Oct 24, 2023

vmasek Oct 25, 2023

matejchalk Oct 25, 2023 •

edited

Loading

Tlacenka Oct 25, 2023

matejchalk Oct 24, 2023

BioPhoton left a comment

BioPhoton Oct 24, 2023

Tlacenka Oct 25, 2023

matejchalk Oct 25, 2023

BioPhoton Oct 24, 2023

Tlacenka Oct 25, 2023 •

edited

Loading

BioPhoton Oct 24, 2023

Tlacenka Oct 25, 2023

BioPhoton Oct 24, 2023

Tlacenka Oct 25, 2023

Tlacenka commented Oct 27, 2023

		args: [auditOutput, '>', toUnixPath(outputFilePath)],
		outputFile: toUnixPath(outputFilePath),

		import { AuditOutputs, PluginConfig } from '@code-pushup/models';
		import { createFileWriteRunnerConfig } from '@code-pushup/utils';

fix(testing): fix tests on windows #131

fix(testing): fix tests on windows #131

Conversation

Tlacenka commented Oct 18, 2023 • edited Loading

matejchalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejchalk Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BioPhoton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tlacenka Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tlacenka commented Oct 27, 2023

Tlacenka commented Oct 18, 2023 •

edited

Loading

matejchalk Oct 25, 2023 •

edited

Loading

Tlacenka Oct 25, 2023 •

edited

Loading