rtfeldman / node-test-runner

Runs elm-test suites from Node.js. Get it with npm install -g elm-test
BSD 3-Clause "New" or "Revised" License
134 stars 79 forks source link

Add a command to use master version of elm-explorations/test #592

Closed Janiczek closed 2 years ago

Janiczek commented 2 years ago

This is based on a discussion in the Incremental Elm #elm-test Discord channel.

The intent is to allow people to help us beta-test the elm-explorations/test 2.0.0 release and use the master version of elm-explorations/test on their codebases (masqueraded as the 1.2.2 version).

cc @harrysarson

https://user-images.githubusercontent.com/149425/167733507-6aa73ed7-0a30-4102-9d15-26a3fc891ef2.mp4

Janiczek commented 2 years ago

From Discord:

I think we should use a different elm home but is not a strong feeling

I think there's a lot of other small things to think about, like more alerting color for the warning, and cross-compatible ways to do the shell commands, description of the flag, etc...

gampleman commented 2 years ago

I wonder if this could even take an argument, so that it could be used to test locally other branches?

Janiczek commented 2 years ago

@gampleman My initial assumption when hearing @harrysarson's idea was that this would later be reverted, and not a long-term flag. If it was to be longterm, I agree customizing it could be a nice addition.

Janiczek commented 2 years ago

I've incorporated your feedback @lydell although I'm sure my implementation will need some more improvements. It works on my Mac machine but I haven't tried the tar "Windows" path.

harrysarson commented 2 years ago

I wonder if this could even take an argument, so that it could be used to test locally other branches? @Janiczek Janiczek commented 4 days ago @gampleman My initial assumption when hearing @harrysarson's idea was that this would later be reverted, and not a long-term flag. If it was to be longterm, I agree customizing it could be a nice addition.

I was thinking of this being something we revert, but if we end up liking I could see us building it out

lydell commented 2 years ago

@Janiczek Let me know if you need help getting flow, eslint or prettier to pass.

Janiczek commented 2 years ago

Hey @lydell, thanks for the review and your offer! I was a bit swamped with other work, didn't even notice your last few comments 😅 I hope to take a look at this tomorrow and get the PR into shape.

Janiczek commented 2 years ago

This now works via a pair of commands:

lydell commented 2 years ago

@Janiczek I fixed the CI failure, but since you used your master branch I don’t seem to be able to push it…

Edit: I made a pull request to your pull request: https://github.com/Janiczek/node-test-runner/pull/1

lydell commented 2 years ago

This is looking really good now!

I finally tested on Windows, and unfortunately I ran into an error:

PS C:\Users\Simon\stuff\node-test-runner\example-application> node ..\bin\elm-test install-unstable-test-master
Using the master version of elm-explorations/test in place of 1.2.2.
Note: You will need to use the `elm-test uninstall-unstable-test-master` command afterwards to get back to the 1.2.2 version.
Removing C:\Users\Simon\stuff\node-test-runner\example-application\elm-stuff\generated-code\elm-community\elm-test\0.19.1-revision7
Removing C:\Users\Simon\AppData\Roaming\elm\0.19.1\packages\elm-explorations\test\1.2.2
Downloading https://codeload.github.com/elm-explorations/test/zip/refs/heads/master
Unzipping C:\Users\Simon\stuff\node-test-runner\example-application\elm-stuff\generated-code\elm-community\elm-test\0.19.1-revision7\elm-explorations-test.zip
Moving to ELM_HOME: C:\Users\Simon\AppData\Roaming\elm\0.19.1\packages\elm-explorations\test\1.2.2
EPERM: operation not permitted, rename 'C:\Users\Simon\stuff\node-test-runner\example-application\elm-stuff\generated-code\elm-community\elm-test\0.19.1-revision7\test-master' -> 'C:\Users\Simon\AppData\Roaming\elm\0.19.1\packages\elm-explorations\test\1.2.2'
PS C:\Users\Simon\stuff\node-test-runner\example-application>

Not sure why that happens yet.

lydell commented 2 years ago

I figured it out! Made another PR here: https://github.com/Janiczek/node-test-runner/pull/2/files

lydell commented 2 years ago

One final final question 😅 What way is the easiest to verify that:

lydell commented 2 years ago

Replying to myself, I figured out an easy way to test: I added Debug.log "a" Fuzz.andThen and checked if the tests resulted in a compilation error or a print. It works!

However, I first thought it didn’t work, because I tried with example-application/ in this repo, and it used elm-test 1.2.0 so nothing happened. Is it worth doing anything about that? Checking the version in elm.json (sounds annoying to code). Wait – I just noticed our prints do mention that we’re only replacing version 1.2.2 so maybe that’s good enough? Especially since 1.2.2 was released June 29, 2019 so all real tests out there probably use the latest version anyway?

Here’s where 1.2.2 is mentioned multiple times: ``` ❯ ../bin/elm-test install-unstable-test-master Using the master version of elm-explorations/test in place of 1.2.2. Note: You will need to use the `elm-test uninstall-unstable-test-master` command afterwards to get back to the 1.2.2 version. Removing /Users/lydell/forks/node-test-runner/example-application/elm-stuff/generated-code/elm-community/elm-test/0.19.1-revision7 Removing /Users/lydell/.elm/0.19.1/packages/elm-explorations/test/1.2.2 Downloading https://codeload.github.com/elm-explorations/test/zip/refs/heads/master Unzipping /Users/lydell/forks/node-test-runner/example-application/elm-stuff/generated-code/elm-community/elm-test/0.19.1-revision7/elm-explorations-test.zip Moving to ELM_HOME: /Users/lydell/.elm/0.19.1/packages/elm-explorations/test/1.2.2 Removing /Users/lydell/forks/node-test-runner/example-application/elm-stuff/generated-code/elm-community/elm-test/0.19.1-revision7/elm-explorations-test.zip ```

EDIT: Added a check in https://github.com/Janiczek/node-test-runner/pull/3. It was easy enough.

Janiczek commented 2 years ago

Good to see you figured how to test this out:

What way is the easiest to verify that: I actually get the master version. I was able to back to the published version.

But anyways just to post my way of testing this, I've simply ran this local version of elm-test in one of my repos that had Shrink.noShrink in its tests/ files.

So it's usually fine to do something like

cd ~/Localhost/elm/elm-secret-sharing
../../cloned/node-test-runner/bin/elm-test # 1.2.2, works
../../cloned/node-test-runner/bin/elm-test install-unstable-test-master
../../cloned/node-test-runner/bin/elm-test # master, doesn't work
../../cloned/node-test-runner/bin/elm-test uninstall-unstable-test-master
../../cloned/node-test-runner/bin/elm-test # 1.2.2, works
lydell commented 2 years ago

I noticed that the intellij-elm plugin does not like when I run elm-test install-unstable-test-master. It asks me to “re-attach elm.json”, and when I do it gives an error. Is that something you’ve noticed as well?

Janiczek commented 2 years ago

I noticed that the intellij-elm plugin does not like

I don't use that one (I'm pretty light on Elm editor plugins) so I didn't notice 😬 I don't suppose intellij-elm checks hashes / redownloads archives, so perhaps this is just a matter of stale cache? Would the File | Invalidate Caches button help?

lydell commented 2 years ago

Found the issue and made a PR: https://github.com/Janiczek/node-test-runner/pull/4

Janiczek commented 2 years ago

Those auto-close comments are treacherous 😁