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

Make the test script run on Windows #8

Closed
jfmengels opened this issue Sep 24, 2020 · 2 comments
Closed

Make the test script run on Windows #8

jfmengels opened this issue Sep 24, 2020 · 2 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jfmengels
Copy link
Owner

jfmengels commented Sep 24, 2020

The current test script is

elm make --docs=docs.json && elm-format src/ tests/ --validate && elm-test && elm-review && npm run package-tests -s

But && does not work on Windows, so this prevents Windows users from helping out on this package.

I know of npm-run-all which helps make scripts run on Windows. It does require using npm scripts, so every part will need to be defined in a separate npm script. That has its benefits, but if you can come up with a different solution, let me know.

If we go with using npm-run-all, the task would be to

  • Create a script for every part of the test script

  • Add npm-run-all as a dependency

  • Replace the test script with one like npm-run-all make verify-format ...

  • Make sure the scripts are run sequentially. Running them in parallel would have little performance benefits but make failures quite hard to understand I think.

  • See if everything

  • Optional/bonus: Make the CI run the tests on Windows (and Mac?) to make sure that works. I think we'd need to change the runs-on field for test in .github/workflows/test.yml to [ ubuntu-latest, windows-latest, macos-latest ] but I haven't tried it out. More info here.

The idea is that all of this work, if proved successful, will then be the default setup for all review packages when created by elm-review new-package. That can be a separate PR for jfmengels/node-elm-review, but I think this is a good first step before generalizing.

@fredericbonnet
Copy link
Contributor

Hello, I've just submitted PR #9 that splits the test script into separate subscripts using npm-run-all. I've tested it with success on my Windows machine (some subtests fail on my machine but this has nothing to do with the NPM scripts themselves since the plain command lines produce the same outcome).

I have not implemented the CI part because I have no experience with GitHub CI. I suggest you create a separate issue.

Happy Hacktoberfest!

@jfmengels
Copy link
Owner Author

Fixed in #10. Thanks @fredericbonnet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants