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.

Sign up for GitHub

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

Jump to bottom

Fix user home config conflicting with the test suite #2725

Closed
wants to merge 3 commits into from
Closed

Fix user home config conflicting with the test suite #2725

wants to merge 3 commits into from

Conversation

TimvdLippe
Copy link
Contributor

Summary
The test suite did not properly isolate the home config from
influencing the behavior of the tests. Therefore if a user
used the global configuration and also cloned Yarn, the tests could fail.

A primary example of that was when a user set the prefix value.
If the user would set the prefix, the test suite would fail.

Test plan

  1. Install yarn on your machine following https://yarnpkg.com/en/docs/install
  2. yarn config set prefix /usr/local (or any other location)
  3. Clone yarn
  4. Execute the test suite of yarn

Without this fix, react-native would be installed in /usr/local/bin/ and the test suite fails in __tests__/commands/global.js. Now the prefix is correctly set and react-native is installed in the proper location.

TimvdLippe
TimvdLippe commented Feb 19, 2017
View reviewed changes
src/registries/npm-registry.js Outdated
@@ -98,7 +98,7 @@ export default class NpmRegistry extends Registry {
async getPossibleConfigLocations(filename: string): Promise<Array<[boolean, string, string]>> {
const possibles = [
[false, path.join(this.cwd, filename)],
[true, this.config.userconfig || path.join(userHome, filename)],
[true, this.config.userconfig || path.join(process.env.YARN_HOME_FOLDER || userHome, filename)],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before coming up with this fix, I tried several others. Most notably I tried to pass on the config to override this.config.userconfig used on the same line. However, this.config is initialized as empty object in the constructor of the Registries. Passing on an initialConfig would work, but in cli/commands/global.js this config is discarded and rewritten. Therefore that solution would require passing on config objects all over the place in order to properly inject the location.

Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing because we have HOME defined in constants.js, npm-registry and yarn-registry.
Could you bring all of them into one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the util-home-dir to export a function instead. This allows for later extension if developers want to modify their home dir too. I hope that this was what you were looking for, else please let me know!

@TimvdLippe TimvdLippe force-pushed the fix-home-config-influence branch from 3ab1b0b to ce70e4a Compare February 21, 2017 10:07
@TimvdLippe
Copy link
Contributor Author

Rebased this PR to fix the import test failures, but now AppVeyor is still failing per #2613 (comment)

@TimvdLippe TimvdLippe force-pushed the fix-home-config-influence branch from ce70e4a to 3ca3c32 Compare February 27, 2017 14:00
@TimvdLippe
Copy link
Contributor Author

I rebased this PR on master and all tests are passing now.

@TimvdLippe TimvdLippe force-pushed the fix-home-config-influence branch from 3ca3c32 to 550a74f Compare February 28, 2017 21:18
bestander
bestander reviewed Mar 3, 2017
View reviewed changes
__tests__/commands/_helpers.js Outdated
@@ -9,6 +9,7 @@ import {run as check} from '../../src/cli/commands/check.js';
import * as fs from '../../src/util/fs.js';
import {Install} from '../../src/cli/commands/install.js';
import Config from '../../src/config.js';
import {userHomeDir, setUserHomeDir} from '../../src/util/user-home-dir';
Copy link
Member

Choose a reason for hiding this comment

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

I think for tests this can be achieved with a simple jest mock,

yarn/__tests__/commands/install/integration.js

Line 41 in ed6b9b9

jest.setMock('../../../src/constants', Object.assign(automock, mocks));

Then you won't need to do setter/getter and change the code

bestander
bestander requested changes Mar 3, 2017
View reviewed changes
Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Better mock user-home with Jest

@TimvdLippe TimvdLippe force-pushed the fix-home-config-influence branch from 550a74f to 1f52ca8 Compare March 12, 2017 15:01
@TimvdLippe
Copy link
Contributor Author

@bestander thank you for your suggestion of using jest.mock, I was not aware of this functionality. I took the code of the integration test as example, but sadly I was unsuccesful. For some reason, reimporting global does not correctly use the mocked user-home-dir. I am unsure how I can make this work 😢

Could you maybe point out what the issue is? Thank you in advance!

@bestander
Copy link
Member

@TimvdLippe, you can have a look at https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/integration.js#L38 how an internal js module can be mocked.
I think you are on the right path, what causes you the trouble?

@TimvdLippe
Copy link
Contributor Author

@bestander I have copied that to https://github.com/yarnpkg/yarn/pull/2725/files#diff-ae8f7de5bfa66fccd0d4a651ae01fc77R22

The problem right now is that when I run the test, the home directory does not seem to be mocked. I suspect that the problem is that user-home-dir is loaded by constants which in turn is loaded by various other modules. These modules are not reloaded after mocking, therefore not altering their usage of user-home-dir. Thus mocking for only global does not seem to work. However, I can't seem to figure out how to also reset these other usages. I tried to require all imports of the global testsuite, but that broke like everything 😛

@bestander
Copy link
Member

Yeah, you are right, the value from user-home-dir is imported into other projects.
Would it work if you mock GLOBAL_MODULE_DIRECTORY in constants.js instead?

@TimvdLippe
Fix user home config conflicting with the test suite
fc95ff1 
The test suite did not properly isolate the home config from
influencing the behavior of the tests. Therefore if a user
used the global configuration and also cloned Yarn, the tests could fail.

A primary example of that was when a user set the prefix value.
If the user would set the prefix, the test suite would fail.
@TimvdLippe
Use Jest mock to fix user home location
48e7656
@TimvdLippe
Mock constants global directory instead
d2ce0ad
@TimvdLippe TimvdLippe force-pushed the fix-home-config-influence branch from 1f52ca8 to d2ce0ad Compare April 23, 2017 12:36
@TimvdLippe
Copy link
Contributor Author

The latest commit tries to do so, but sadly still fails locally 😭 Maybe a Jest engineer could chime in to explain why it is not working?

@TimvdLippe TimvdLippe mentioned this pull request May 1, 2017
Merged
@arcanis arcanis closed this in #3238 May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bestander bestander bestander requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TimvdLippe @bestander