feat(core): use user configured npm registry #4937

Tamir-M · 2024-08-06T10:27:07Z

Fix:

Running npm init stryker use default npm registry instead of the one configured in npm config #4936

nicojs

Thanks for the PR. I have some remarks.

packages/core/src/initializer/index.ts

nicojs · 2024-08-09T08:51:20Z

packages/core/src/initializer/npm-client.ts

@@ -65,7 +65,7 @@ export class NpmClient {
      const response = await this.innerNpmClient.get<NpmSearchResult>(path);
      return handleResult(path)(response);
    } catch (err) {
-      this.log.error(`Unable to reach 'https://registry.npmjs.com' (for query ${path}). Please check your internet connection.`, errorToString(err));
+      this.log.error(`Unable to reach '${getRegistry()}' (for query ${path}). Please check your internet connection.`, errorToString(err));


Instead of using getRegistry here, you can simply let the npm registry location be injected, see previous comment about dependency injection

nicojs · 2024-08-09T08:52:00Z

packages/core/src/initializer/index.ts

-const NPM_REGISTRY = 'https://registry.npmjs.com';
+let currentRegistry: string | undefined;
+
+function getRegistry(): string {


Can you add unit tests for this function? Also, move it to a separate file.

I am having some issues with the last case where I would need to use the execaSync, I have a few ways I can test this:

Mocking:
Would result in not actually testing the functionality of it, wouldn't do anything.

Editing the npm config:
This may cause a problem if I do beforeEach and afterEach to edit the npm config there are multiple things that could happen:

If the command fails it can ruin the person's npm configuration.

If the person aborts mid test it could ruin the person's npm configuration.

Seems like too be too many points of failure.
I have no idea how to test this or if I even should, as I gave a default value if it fails and warned them.
I would wait for your input.

Ah yes, great question. Please add 2 more unit tests, 1 for the happy flow and one for the alternate flow using mocking of execaSync. You are right that we won't test the actual command npm config get registry, but I would expect that to work as documented. However, if you want to add a test for that, you are welcome to add an integration test (see test/integration).

To mock out execSync, we can't simply use sinon.stub(execa, 'execSync'), since it isn't a method that's being called, but rather a function that is directly exported. To mock it away, we can use the same approach as we did for execaCommand, see the coreTokens.execa for an example. So you can add a coreTokens.execaSync token.

nicojs · 2024-08-09T08:52:33Z

packages/core/src/initializer/index.ts

+      currentRegistry = 'https://registry.npmjs.com';
+    } else {
+      // Using global as when trying to get the registry inside npm workspace it would fail
+      const registry = execaSync('npm', ['config', 'get', '--global', 'registry'], {


Please also add a try ... catch here, because a lot of stuff can go wrong here. If an error occurs we should log a warning (using the logger that you can inject) and use the default value,

nicojs · 2024-08-09T08:54:04Z

packages/core/src/initializer/index.ts

+        timeout: 20000,
+      });
+
+      currentRegistry = registry.stdout;


Should the result be trimmed here?

Suggested change

currentRegistry = registry.stdout;

currentRegistry = registry.stdout.trim();

move getRegistry to a different file, wrap `execaSync` in try-catch, and use dependency injection, and changed tests to run

Tamir-M · 2024-08-11T17:30:20Z

@nicojs I've applied the changes.

nicojs

Thanks for fixing my review comments. I have some final remarks regarding the tests.

Note: if you need help to create a mock for execSync, feel free to ask, I would be happy to help out.

nicojs · 2024-08-11T21:16:51Z

packages/core/test/unit/initializer/npm-registry.spec.ts

+
+      process.env.npm_command = 'value';
+
+      const registry = getRegistry(log4js.getLogger());


Please use testInjector.logger. It contains a logger mock that is ready to be used. You can verify log messages on it using sinon.asserts.calledOnceWithExactly(testInjector.logger.warn, '...');

nicojs · 2024-08-11T21:17:32Z

packages/core/test/unit/initializer/npm-registry.spec.ts

+
+      process.env.npm_command = 'value';
+
+      const registry = getRegistry(log4js.getLogger());


packages/core/test/unit/initializer/npm-registry.spec.ts

nicojs · 2024-08-11T21:24:39Z

packages/core/test/unit/initializer/stryker-initializer.spec.ts

@@ -55,7 +56,8 @@ describe(StrykerInitializer.name, () => {
    syncBuiltinESMExports();
    sut = testInjector.injector
      .provideValue(initializerTokens.out, out as unknown as typeof console.log)
-      .provideValue(initializerTokens.restClientNpm, npmRestClient)
+      .provideFactory(initializerTokens.npmRegistry, getRegistry)
+      .provideFactory(initializerTokens.restClientNpm, createNpmRegistryClient)


Since this is a unit test for the StrykerInitializer, I wouldn't expect the "real" getRegistry to be used here. Instead, you can keep using provideValue here for the npmRestClient sinon mock here.

Indeed, the tests are failing, see https://github.com/stryker-mutator/stryker-js/actions/runs/10341544430/job/28626286827?pr=4937

…n restClientNpm

…t on npmRegistry

Tamir-M · 2024-08-21T16:35:02Z

@nicojs I've applied the changes you've asked for, would love to get them reviewed.

nicojs

Thanks a lot for implementing the requested changes! I've got small remarks about the use of test hooks and assertions. Other than that this looks good.

nicojs · 2024-08-22T10:32:26Z

packages/core/test/unit/initializer/npm-registry.spec.ts

+    let oldNpmConfigRegistry: string | undefined;
+    let oldNpmCommand: string | undefined;
+
+    before(() => {


This should be beforeEach instead of before (or change afterEach to be after)

nicojs · 2024-08-22T10:33:51Z

packages/core/test/unit/initializer/npm-registry.spec.ts

+      sinon.assert.callCount(execaCommandSyncMock, 1);
+      sinon.assert.calledWith(execaCommandSyncMock, 'npm config get --global registry', {
+        stdout: 'pipe',
+        timeout: 20000,
+      });


Can be simplified to calledOnceWithExactly

nicojs · 2024-08-22T10:36:15Z

packages/core/test/unit/initializer/npm-registry.spec.ts

+      sinon.assert.callCount(execaCommandSyncMock, 1);
+      sinon.assert.calledWith(execaCommandSyncMock, 'npm config get --global registry', {
+        stdout: 'pipe',
+        timeout: 20000,
+      });


…before

Tamir-M · 2024-08-22T10:55:54Z

Thank you for the remarks, I've applied them.

nicojs

LGTM

feat(core): use user configured npm registry

d1224cc

nicojs requested changes Aug 9, 2024

View reviewed changes

Tamir-M added 2 commits August 11, 2024 20:19

chore(core): getRegistry dependency injection

5858a5a

move getRegistry to a different file, wrap `execaSync` in try-catch, and use dependency injection, and changed tests to run

test(core): add unit testing for getRegistry

e0362a6

nicojs requested changes Aug 11, 2024

View reviewed changes

Tamir-M added 4 commits August 12, 2024 09:45

test(core): made it so tests won't fail due to not using sinon mock o…

41deaae

…n restClientNpm

chore(core): made execaSync a part of dependency injection and used i…

f25e12a

…t on npmRegistry

test(core): now doesn't fail old tests after dependency injection

899476f

test(core): mocks execaSync with sinon and add 2 happy path tests

db788a1

nicojs requested changes Aug 22, 2024

View reviewed changes

test(core): simplified sinon matching and used beforeEach instead of …

6c2d00f

…before

nicojs approved these changes Aug 22, 2024

View reviewed changes

nicojs enabled auto-merge (squash) August 22, 2024 11:23

nicojs merged commit 33e348e into stryker-mutator:master Aug 22, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): use user configured npm registry #4937

feat(core): use user configured npm registry #4937

Tamir-M commented Aug 6, 2024

nicojs left a comment

nicojs Aug 9, 2024

nicojs Aug 9, 2024

Tamir-M Aug 11, 2024

nicojs Aug 11, 2024 •

edited

Loading

nicojs Aug 9, 2024

nicojs Aug 9, 2024

Tamir-M commented Aug 11, 2024

nicojs left a comment •

edited

Loading

nicojs Aug 11, 2024

nicojs Aug 11, 2024

nicojs Aug 11, 2024

Tamir-M commented Aug 21, 2024

nicojs left a comment

nicojs Aug 22, 2024

nicojs Aug 22, 2024

nicojs Aug 22, 2024

Tamir-M commented Aug 22, 2024

nicojs left a comment

	currentRegistry = registry.stdout;
	currentRegistry = registry.stdout.trim();


		process.env.npm_command = 'value';

		const registry = getRegistry(log4js.getLogger());

feat(core): use user configured npm registry #4937

feat(core): use user configured npm registry #4937

Conversation

Tamir-M commented Aug 6, 2024

nicojs 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

nicojs Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tamir-M commented Aug 11, 2024

nicojs left a comment • 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

Tamir-M commented Aug 21, 2024

nicojs 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

Tamir-M commented Aug 22, 2024

nicojs left a comment

Choose a reason for hiding this comment

nicojs Aug 11, 2024 •

edited

Loading

nicojs left a comment •

edited

Loading