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

Update and test pnpm behavior so that we simulate a new user install #958

Conversation

NullVoxPopuli
Copy link
Contributor

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #958 (2a9a5ef) into master (c590272) will decrease coverage by 0.17%.
The diff coverage is 75.00%.

❗ Current head 2a9a5ef differs from pull request most recent head 27c856a. Consider uploading reports for the commit 27c856a to get more accurate results

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
- Coverage   94.54%   94.38%   -0.17%     
==========================================
  Files          17       17              
  Lines         550      552       +2     
==========================================
+ Hits          520      521       +1     
- Misses         30       31       +1     
Impacted Files Coverage Δ
lib/utils/dependency-manager-adapter-factory.js 91.30% <ø> (ø)
lib/dependency-manager-adapters/pnpm.js 89.41% <75.00%> (-0.95%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kategengler
Copy link
Member

I wonder if we should make this new behavior have an opt-in or out?

Users have the capability to override --ignore-lockfile for the other managers if they do want to test with a lockfile.

@NullVoxPopuli
Copy link
Contributor Author

that's a good point 🤔

are you thinking it'd be like, a top-level config option like usePnpm?

like, maybe:

{
  usePnpm: true,
  adapterOptions: { // specific to each adapter, with their own defaults
    keepLockfile: true, // default false
  }

}

idk

@kategengler
Copy link
Member

@NullVoxPopuli I think that's a bit too close to buildManagerOptions. I think it should be opt-out, same as the other managers (though theirs are through flags). I don't really love adding more top-level config, but maybe we should here: floatDependencies: true. We could make it work for the other managers too, via their command line flags, though it would interact unfortunately with buildManagerOptions and npmOptions.

Alternatively, I think there's some precedent in the repo for making our own flags that can be supplied in npmOptions or buildManagerOptions that we remove before passing them along.

expect(await fs.readFile('pnpm-lock.ember-try.yaml', 'utf-8')).to.equal(
'originalYAML: true\n'
);
expect(fs.existsSync('.node_modules.ember-try')).to.be.false;
});

it('copies up the `pnpm-lock.yaml` file when pnpmUseLockfile is true (non-default)', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the inverse of this test is covered by the test above

* @property {boolean} [useWorkspaces]
* @property {boolean} [useYarn]
* @property {boolean} [usePnpm]
* @property {boolean} [pnpmUseLockfile] default false - re-uses the lockfile, which would test minimum supported versions in the project. When false, we simulate a "new project install"
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to document this in the README?

@NullVoxPopuli
Copy link
Contributor Author

Going to do a separate PR that uses this:
image

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.

None yet

2 participants