Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): add support for pnpm as package manager #3802

Merged

Conversation

AbdelHajou
Copy link
Contributor

@AbdelHajou AbdelHajou commented Oct 22, 2022

stryker init command now has the option to use pnpm instead of npm as package manager.

image

Resolves #3448 and closes #3221 (duplicates)

Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Looks good, also with the additional test 🧡

There is one big showstopper left. We need to explicitly specify plugins here, since the automatic plugin detection fails with pnpm's installation model.

So when the @stryker-mutator/jest-runner gets installed, it should also be added to the plugins array in the stryker.conf.json file. Would you feel comfortable to add that as well?

Actually, this should also be added to troubleshooting.md.

packages/core/src/initializer/stryker-initializer.ts Outdated Show resolved Hide resolved
@AbdelHajou
Copy link
Contributor Author

Thanks for the review @nicojs , I'm working on it. Is it true that during stryker init it's unknown whether the project uses TypeScript, so it can't be added to the plugins array automatically? I guess this is where the Troubleshooting docs will come in handy.

@AbdelHajou AbdelHajou requested a review from nicojs October 25, 2022 16:48
Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @AbdelHajou. This looks great! You even documented the issue in troubleshooting.md 👍

I've got a remark on the troubleshooting. You can reproduce the issue using these repro steps:

npm i -g pnpm
mkdir tmp && cd tmp
pnpm init --yes
pnpm add -D mocha @stryker-mutator/core @stryker-mutator/mocha-runner
mkdir src
mkdir test
touch src/math.js
# export function add(a, b) {
#  return a + b;
# }

touch test/math.spec.js
# import { add } from '../src/math.js';
# import assert from 'node:assert/strict';
# 
# describe('add', () => {
#    it('should return 42 for 42 and 0', () => {
#        assert.equal(add(42, 0), 42);
#    });
# });

code package.json
# Add "type": "module"
npx mocha

#  add
#    ✔ should return 42 for 42 and 0

touch stryker.conf.json
# {
#     "$schema": "./node_modules/@stryker-mutator/core/schema/stryker-schema.json",
#     "testRunner": "mocha"
# }
npx stryker

# see the error


**Symptom**

Running mutation tests takes longer than expected when using pnpm in combination with TypeScript.
Copy link
Member

@nicojs nicojs Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem isn't that mutation testing takes too long; the problem is this when running Stryker:

Error: Could not inject [class ChildProcessTestRunnerWorker]. Cause: Cannot find TestRunner plugin "mocha". No TestRunner plugins were loaded.

The @stryker-mutator/typescript-checker doesn't improve performance; it dramatically decreases it, in favor of mutation report quality. I wouldn't name typescript at all for this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I've updated the docs. I still mention typescript (in the form of typescript-checker) because this should be the only plugin causing trouble for newer projects. The others should be added by stryker init. If you have any more feedback I'll be happy to process it during the hackathon tomorrow! 🤠

@nicojs nicojs enabled auto-merge (squash) October 27, 2022 20:19
@nicojs nicojs merged commit af0e34e into stryker-mutator:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add pnpm as a package manager? Support for pnpm as a package manager
2 participants