-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add test for isFakeRoot #4435
Add test for isFakeRoot #4435
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the quick follow up!
__tests__/util/root-user.js
Outdated
|
||
test('isRootUser', () => { | ||
expect(isRootUser(null)).toBe(false); | ||
expect(isRootUser(1001)).toBe(false); | ||
expect(isRootUser(0)).toBe(true); | ||
}); | ||
|
||
test('isFakeRoot', () => { | ||
delete process.env.FAKEROOTKEY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should explicitly set this to ''
or something since deleting would just restore it to the default value if the system running this. So if the tests were run with fakreoot
then deleting that env variable would just reset it to the system value.
Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out, and delete process.env.FAKEROOTKEY
removes the key from the object.
Running the tests with fakeroot
actually works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I've tested locally to see the same result. That said be advised that deleting it like this actually removes any existing value without any way for restoring it so your test is actually affecting the global state permanently.
Are you sure you want to do that?
This change will increase the build size from 9.52 MB to 9.52 MB, an increase of 1.07 KB (0%)
|
1 similar comment
This change will increase the build size from 9.52 MB to 9.52 MB, an increase of 1.07 KB (0%)
|
**Summary** Follow up to yarnpkg#4431. `isFakeRoot` didn't have any tests and it was broken from the start. yarnpkg#4431 solved it and it was merged to be included in 1.0.2 without tests. This patch adds the missing tests for this function. **Test plan** Added new tests, duh :D
This adds a test for
isFakeRoot()
as proposed by @BYK in #4431.