cucumber / cucumber-js

Cucumber for JavaScript
https://cucumber.io
MIT License
5.04k stars 1.09k forks source link

Error message when running cucumber test with npm prefix on Windows #2274

Closed arbughiu closed 1 year ago

arbughiu commented 1 year ago

👓 What did you see?

There is a bug causing "It looks like you're running Cucumber from a global installation." message to occur when the following conditions occur:

✅ What did you expect to see?

No error message should occur on Windows, if the --prefix directory is correct.

📦 Which tool/library version are you using?

latest

🔬 How could we reproduce it?

Please see an example of the bug that can be easily replicated here: https://github.com/arbughiu/cucumber-windows-npm-prefix

michael-lloyd-morris commented 1 year ago

Hmm... I'll look into this.

michael-lloyd-morris commented 1 year ago

Ok, I don't fully understand the way we are doing this check, but it looks a bit hackish. I found this library

https://www.npmjs.com/package/resolve-package-path

This might resolve both cases correctly, but do we want to add this package to fix this somewhat rare corner case?

davidjgoss commented 1 year ago

To be clear this is only a warning, doesn't prevent you running. The detection is from https://www.npmjs.com/package/is-installed-globally and I don't think it's something we should implement/maintain in Cucumber. Since https://github.com/cucumber/cucumber-js/pull/2089 the handling for the issue that global (and other invalid) installations cause is much better, so arguably we could make this warning more subtle (one line, softer language) or even just remove it.

arbughiu commented 1 year ago

@davidjgoss yes I agree its only a warning, and its not affecting behaviour, making this a minor bug. It's more of an annoyance right now, as the QAs running test scripts on Windows in my current company are asking me what they've done wrong to cause that warning message to occur. (the warning does not occur on Mac). Our setup is a bit more complex, but I've simplified it greatly in the example repo I provided.

michael-lloyd-morris commented 1 year ago

If we remove it and there is a crash because cucumber was installed as a global, will the error message be clear? Could it be altered to make it clear in such circumstances? If so I would suggest doing that - remove this warning then alter the error message that arises when trying to import or require a step file when cucumber is installed globally.

That said, I have a feeling this might be very difficult to write a test case for.

arbughiu commented 1 year ago

FYI I am not proposing we remove anything (as it would still be useful to be notified of a global installation) but rather fix the original intended behaviour.

In my sample repo we can see the following:

On Mac: npm run test - runs fine ✅ npm --prefix . run test - runs fine ✅

On Windows: npm run test - runs fine ✅ npm --prefix . run test - shows the error message above ❌

But npm --prefix . run test should produce the same result in both Windows and Mac.

The issue appears to be inside the is-installed-globally dependency

michael-lloyd-morris commented 1 year ago

The issue is Windows.

And I'm saying this as a Windows user.

Kidding aside, the most direct fix would need to be in that is-installed-globally package. All we can do at the Cucumber level is find another approach.

davidjgoss commented 1 year ago

Took the middle ground here - the warning remains but the language is softened and it's only shown in debug mode. The issues that stem from using a global install are much less cryptic than they used to be, so I think this is fine now.

https://github.com/cucumber/cucumber-js/pull/2285

davidjgoss commented 1 year ago

(I think it would be worth raising an issue on is-installed-globally all the same though.)

davidjgoss commented 1 year ago

Released in https://github.com/cucumber/cucumber-js/releases/tag/v9.1.2