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

Add test for global add with prefix #1919

Closed
wants to merge 4 commits into from

Conversation

torifat
Copy link
Member

@torifat torifat commented Nov 17, 2016

Summary

Test case for global add <package> --prefix <path>

Covers #1877

@onemen
Copy link

onemen commented Nov 18, 2016

This PR doesn't fix the issue of add-without-flag test.
The test installs react-native to the user global bin folder

After yarn global add react-native-cli this is the content of the file react-native in my bin folder

#!/bin/sh
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")

case `uname` in
    *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
esac

if [ -x "$basedir//bin/sh" ]; then
  "$basedir//bin/sh"  "$basedir/config/global/node_modules/.bin/react-native" "$@"
  ret=$?
else 
  /bin/sh  "$basedir/config/global/node_modules/.bin/react-native" "$@"
  ret=$?
fi
exit $ret

After jest __tests__\commands\global.js the original file replaced with this one.
This breaks my react-native when the temp folder no longer exist.

#!/bin/sh
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")

case `uname` in
    *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
esac

if [ -x "$basedir//bin/sh" ]; then
  "$basedir//bin/sh"  "$basedir/../Temp/yarn-add-without-flag-0.8375984502260758/.yarn/.global/node_modules/.bin/react-native" "$@"
  ret=$?
else 
  /bin/sh  "$basedir/../Temp/yarn-add-without-flag-0.8375984502260758/.yarn/.global/node_modules/.bin/react-native" "$@"
  ret=$?
fi
exit $ret

As you can see in #1918, i solved this issue by adding

+  // set global bin
+  if (!flags.prefix) {
+    flags.prefix = path.join(cwd, '.yarn', '.global', '.bin');
+  }

to runGlobal function. to make sure that all global test will NOT Use the global bin folder

@torifat
Copy link
Member Author

torifat commented Nov 18, 2016

This PR doesn't fix the issue of add-without-flag test.

@onemen I'm confused. Why this PR need to fix it? And, I also didn't mention anywhere that it does. Also, your PR and this PR is mutually exclusive. Or I'm missing anything here?

After yarn global add react-native-cli this is the content of the file react-native in my bin folder

I have already told you in another place that the test I wrote is not an ideal one and doesn't cover everything. That was a basic guard so that we don't ship global completely broken, like we did in v0.17.0.

And, similarly this PR to address just a specific bug, just to prevent regression.

As you can see in #1918, i solved this issue by adding

I tried your PR locally and it fails.
screenshot 2016-11-18 13 21 55

@torifat
Copy link
Member Author

torifat commented Nov 18, 2016

After jest tests\commands\global.js the original file replaced with this one.
This breaks my react-native when the temp folder no longer exist.

This is a valid concern. So, please fix your PR so that we can merge it.

@onemen
Copy link

onemen commented Nov 18, 2016

I tried your PR locally and it fails.

That's odd, in my local fork it pass

yarn>jest __tests__\commands\global.js > nul
 PASS  __tests__\commands\global.js (9.562s)
  √ add without flag (7186ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        10.034s
Ran all test suites matching "__tests__\commands\global.js".

I'm on windows 10 the files copy to
C:\Users\Onemen\AppData\Local\Temp\yarn-add-without-flag-0.39320614322312464\.yarn\.global\.bin

@torifat torifat force-pushed the test/global-add-prefix branch from a9482ad to dd66858 Compare November 18, 2016 20:09
test.concurrent('add with prefix flag', async (): Promise<void> => {
await fs.unlink(tmpGlobalFolder);
return runGlobal('add', {prefix: tmpGlobalFolder}, ['react-native-cli'], 'add-with-prefix-flag', async (config) => {
assert.ok(await fs.exists(path.join(tmpGlobalFolder, 'bin', 'react-native')));
Copy link

Choose a reason for hiding this comment

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

See the modification I have made in #1918 to fix the path on different platform
Change this line accordingly:
- assert.ok(await fs.exists(path.join(tmpGlobalFolder, 'bin', 'react-native')));
+ assert.ok(await fs.exists(path.join(getBinFolder(tmpGlobalFolder), 'react-native')));

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@torifat torifat force-pushed the test/global-add-prefix branch from dd66858 to b2a65b9 Compare November 22, 2016 15:07
@torifat torifat force-pushed the test/global-add-prefix branch from b2a65b9 to 7b5052a Compare November 24, 2016 12:01
@bestander
Copy link
Member

Hi, @torifat.
Sorry I was busy tracking down some 0.17 issues.
This one is on my list.
Do you still want it to be merged?

@torifat
Copy link
Member Author

torifat commented Nov 24, 2016

@bestander No. Because of #2007 I have closed this one and created a new PR at #2025.

@bestander
Copy link
Member

Cool, I'll get to it as soon as I clear some debts

@torifat
Copy link
Member Author

torifat commented Nov 24, 2016

It failed 😄 I have to make it pass first.

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.

3 participants