Fix test failures on windows #313
Conversation
The "npm" executable can not be found on windows. Using "npm.cmd" instead of "npm" fixes the tests. - Added two constants for *nix and windows executables and use the fitting one after running an os detection. - Tests adjusted to reflect the npm command string change. Fix jhipster#312
|
Hey @SudharakaP and @atomfrede, Could you have a look at this? I wanted to look into the sonar issues of the project but stumbled upon this problem. |
There was a problem hiding this comment.
Hey @SudharakaP and @atomfrede,
Could you have a look at this? I wanted to look into the sonar issues of the project but stumbled upon this problem.
Apparently even the pipeline needs approval from a maintainer to run....
@nomuna: Firstly, sorry for taking a long time to reply. Somehow I missed this one. Now I took a look but I cannot test this PR as I don't have a Windows machine. On the other hand I am a bit hesitant on trying to support Windows since as far as I know most people in the core team doesn't use Windows and thus we will have a maintainability problem when we try to support Windows commands. One can always use WSL (Windows Subsystem for Linux) instead of trying to run on Windows natively. 🤔
@pascalgrimaud : What is your take on this? 🤔
| @@ -62,7 +66,9 @@ public JHipsterService(LogsService logsService, ApplicationProperties applicatio | |||
|
|
|||
| public void installNpmDependencies(String generationId, File workingDir) throws IOException { | |||
| this.logsService.addLog(generationId, "Installing the JHipster version used by the project"); | |||
| this.runProcess(generationId, workingDir, "npm", "install", "--ignore-scripts", "--package-lock-only"); | |||
| final String os = System.getProperty("os.name"); | |||
| final String command = os.contains("indows") ? NPM_WIN : NPM_NIX; | |||
There was a problem hiding this comment.
@nomuna : Why is the string "indows" instead of "windows" ? 🤔
There was a problem hiding this comment.
Will still work if it is "Windows" or "windows" :D
There was a problem hiding this comment.
Besides, this method is only used / called by the test.... Apparently unnecessary even.
For the application to generate a working zip file under windows the jhipster command has to be set to jhipster.cmd
in application-*.yml.
So if the installNpmDependencies method is necessary I would suggest that npm command path is also made configurable through application-*.yml so that the executable path can be set to npm.cmd.
Then the OS detection code can be removed from the method.
|
I think the project should build and work on Windows, specially if there is few code to update. Anyway, this PR is not enought, we should rework all path, using File.separator instead of hard coding /x/y/z So I would say, let do not merge it for now, with the release 7.2.0 |
|
I added this code to my local version to make it work on windows so that I could have a look at the sonar issues. I thought there was a windows pipeline already set up and would trigger on PRs. |
- hard coded, os specific path string replaced with os-independent version in GeneratorService and GeneratorServiceTest - removed unused imports in GeneratorServiceTest
Just pushed a new commit where I replaced all the hard coded / linux paths with OS - independent versions. |
- extended ApplicationProperties for npm process configuration - the default value for tempDir in ApplicationProperties is the OS spec. temporary folder - removed OS detection code from JHipsterService - OS detection and npm path adjustment done to ApplicationProperties before each test
|
I also can't test easily - maybe we could test GitHub Actions with Windows, I need to check as I think that's an option. Then I think this is important to merge (and many thanks to @nomuna for finding & fixing this!). I had the exact same issue on another project, and as I coded both I probably did the same kind of error with the file separators. So it's all my fault! |
The "npm" executable can not be found on windows. Using "npm.cmd" instead of "npm" fixes the tests.
Fix #312