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

fix: make npm run test scripts work cross-environment (#10) #11

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

ms14981
Copy link
Contributor

@ms14981 ms14981 commented Jan 31, 2023

No description provided.

Copy link
Contributor

@gkocak-scottlogic gkocak-scottlogic left a comment

Choose a reason for hiding this comment

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

Before this change, the test and test-generators entry points were incompatiable for cmd and mingw (works fine on regular unix including wsl) due to the execution strategy including subfolders. Usage of cross-env addresses this issue. But we should keep in mind that cross-env is now in maintanance mode.

@ColinEberhardt
Copy link
Collaborator

due to the execution strategy including subfolders

Can you elaborate on this? I'd have thought that if we use the path API, to ensure folders are traversed in cross-platform fashion, this wouldn't be an issue?

@gkocak-scottlogic
Copy link
Contributor

Can you elaborate on this?

For the mentioned entry points, ./*/ syntax to access current/subfolder executables are not working correctly on mingw (git bash) while it's okay on unix-land.

package.json Outdated
@@ -7,8 +7,8 @@
],
"scripts": {
"prepare": "husky install",
"test": "\"./node_modules/.bin/cucumber-js\" -p default",
"test:generators": "\"./node_modules/.bin/cucumber-js\" -p generators",
"test": "cross-env-shell ./node_modules/.bin/cucumber-js -p default",
Copy link
Member

Choose a reason for hiding this comment

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

It's worth reviewing the docs for the npm run command -

In addition to the shell's pre-existing PATH, npm run adds node_modules/.bin to the PATH provided to scripts. Any binaries provided by locally-installed dependencies can be used without the node_modules/.bin prefix. For example, if there is a devDependency on tap in your package, you should write:

"scripts": {"test": "tap test/*.js"}

instead of

"scripts": {"test": "node_modules/.bin/tap test/*.js"}

I believe that would remove the need for cross-env and fix this bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like exactly what we need. Thanks :) I've updated the PR(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

There can be a side note about the hardcoded usage of node_modules/.bin as well.

Someone might have a custom setup for the node_modules folder location. Currently, test entry points make the assumption of node_modules is in its default place. npm bin command provides the path of locally installed binaries dynamically.

Changing from

"test": "node_modules/.bin/cucumber-js -p default"

to

"test": "$(npm bin)/cucumber-js -p default"

addresses this and works fine on unix but I'm not sure about its cross-compatibility.

@ColinEberhardt ColinEberhardt merged commit e107cfd into ScottLogic:main Feb 1, 2023
@github-actions
Copy link

github-actions bot commented Feb 1, 2023

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ms14981 ms14981 deleted the fix-10 branch February 2, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants