Add a command to use master version of elm-explorations/test by Janiczek · Pull Request #592 · rtfeldman/node-test-runner

Janiczek · 2022-05-10T16:22:22Z

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

vid.mp4

Janiczek · 2022-05-10T16:22:49Z

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 · 2022-05-11T10:00:43Z

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

Janiczek · 2022-05-11T14:24:37Z

@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.

lib/Install.js

Janiczek · 2022-05-11T16:56:06Z

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.

lydell

TODO:

Make CI pass
Test manually on Windows (I think spawn.sync throws an error rather than returning a non-zero exit code if unzip does not exist). EDIT: No, the approach with the exit codes works, at least on macOS. So it’s just about verifying that it works on Windows.

lib/Install.js

lib/elm-test.js

harrysarson

I am happy to delegate detail review to others, the shape of this PR looks great!

Given this is a tool to test the new elm-explorations which should be uniform across platforms (its elm) I would think we need windows support (although it would obviously be cool to have).

harrysarson · 2022-05-15T21:33:19Z

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

lib/elm-test.js

lib/Install.js

lydell · 2022-05-16T17:38:56Z

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

Janiczek · 2022-05-25T12:52:23Z

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 · 2022-05-26T20:11:25Z

This now works via a pair of commands:

elm-test install-unstable-test-master
elm-test uninstall-unstable-test-master

lydell · 2022-05-28T08:20:00Z

@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

Various fixes

lib/Install.js

lydell · 2022-05-28T16:05:28Z

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 · 2022-05-28T16:45:12Z

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

Make it work on Windows

lydell · 2022-05-28T17:17:36Z

lib/Install.js

+    });
+
+    // The destination stream is ended by the time it's called
+    file.on('finish', () => resolve(fileInfo));


We seem to resolve the promise with some data we never use here.

lydell · 2022-05-28T17:19:24Z

lib/Install.js

  return 'SuccessfullyInstalled';
 }

+async function downloadFileNative(url, filePath) {


Maybe we should add type annotations to all new functions in this file – for parameters and return types.

Went ahead and did the last final touches: https://github.com/Janiczek/node-test-runner/pull/3

lydell · 2022-05-28T18:00:46Z

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

I actually get the master version.
I was able to back to the published version.

lydell · 2022-05-29T08:52:53Z

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 · 2022-05-29T09:23:27Z

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.

1.2.2 will show no error
master will say the Shrink module doesn't exist

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

Final touches

lydell · 2022-05-30T15:28:01Z

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 · 2022-05-31T09:05:59Z

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 · 2022-06-06T11:19:59Z

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

Install package elm-explorations/test after uninstalling master

Janiczek · 2022-06-06T11:41:05Z

Those auto-close comments are treacherous 😁

Add a flag to use master version of elm-explorations/test

202a2dc

lydell reviewed May 11, 2022

View reviewed changes

lib/Install.js Outdated Show resolved Hide resolved

lib/Install.js Outdated Show resolved Hide resolved

lib/Install.js Outdated Show resolved Hide resolved

Janiczek added 2 commits May 11, 2022 17:50

Small fixes (we'll rip these out anyways)

f8429a3

Use Node versions of commands

14dd7fa

lydell reviewed May 14, 2022

View reviewed changes

lib/Install.js Show resolved Hide resolved

lib/elm-test.js Outdated Show resolved Hide resolved

lib/elm-test.js Outdated Show resolved Hide resolved

harrysarson approved these changes May 15, 2022

View reviewed changes

Janiczek added 2 commits May 16, 2022 08:29

Revert await for Install.install

ae25add

Revert Node 12->14 changes

0d5d3c2

lydell reviewed May 16, 2022

View reviewed changes

lib/elm-test.js Outdated Show resolved Hide resolved

lydell reviewed May 16, 2022

View reviewed changes

lib/Install.js Outdated Show resolved Hide resolved

lydell reviewed May 16, 2022

View reviewed changes

lib/Install.js Outdated Show resolved Hide resolved

Janiczek added 4 commits May 26, 2022 21:54

Apply CR suggestions

f8e8670

Minimize diff

b9bc09a

lint

6a2c471

prettier + bugfix

250a6b5

Janiczek requested a review from lydell May 26, 2022 20:09

Janiczek added 2 commits May 26, 2022 22:10

Fix comments

bec1562

Fix comment again

59818ea

Janiczek changed the title ~~Add a flag to use master version of elm-explorations/test~~ Add a command to use master version of elm-explorations/test May 26, 2022

Fix ?? not available in Node.js 12, the ambitious way

7247fa5

Handle errors like we do in other commands

557634f

lydell and others added 2 commits May 28, 2022 10:26

Replace rmFileSync with fs.unlinkSync

bbf9fa8

Merge pull request #1 from lydell/install-master

7ecf9ff

Various fixes

lydell reviewed May 28, 2022

View reviewed changes

lib/Install.js Outdated Show resolved Hide resolved

Remove mention of rm -rf

bf7150e

Try to fix Windows

94dedda

Merge pull request #2 from lydell/install-master

c11a29d

Make it work on Windows

lydell reviewed May 28, 2022

View reviewed changes

Remove unused value and add type annotations

0b9a0f3

Validate that 1.2.2 is used

54f7093

lydell mentioned this pull request May 29, 2022

0.19.1-revision8 Changelog #560

Closed

5 tasks

Merge pull request #3 from lydell/install-master

1714149

Final touches

lydell and others added 2 commits June 6, 2022 13:22

Install package elm-explorations/test after uninstalling master

e79119f

Merge pull request #4 from lydell/install-master

1815420

Install package elm-explorations/test after uninstalling master

Janiczek closed this Jun 6, 2022

Janiczek reopened this Jun 6, 2022

lydell approved these changes Jun 6, 2022

View reviewed changes

lydell merged commit 768ab12 into rtfeldman:master Jun 6, 2022

Conversation

Janiczek commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Janiczek commented May 10, 2022

Uh oh!

gampleman commented May 11, 2022

Uh oh!

Janiczek commented May 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Janiczek commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harrysarson left a comment

Choose a reason for hiding this comment

Uh oh!

harrysarson commented May 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lydell commented May 16, 2022

Uh oh!

Janiczek commented May 25, 2022

Uh oh!

Janiczek commented May 26, 2022

Uh oh!

lydell commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lydell commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented May 28, 2022

Uh oh!

lydell May 28, 2022

Choose a reason for hiding this comment

Uh oh!

lydell May 28, 2022

Choose a reason for hiding this comment

Uh oh!

lydell May 29, 2022

Choose a reason for hiding this comment

Uh oh!

lydell commented May 28, 2022

Uh oh!

lydell commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Janiczek commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented May 30, 2022

Uh oh!

Janiczek commented May 31, 2022

Uh oh!

lydell commented Jun 6, 2022

Uh oh!

Janiczek commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Janiczek commented May 10, 2022 •

edited

Loading

Janiczek commented May 11, 2022 •

edited

Loading

lydell left a comment •

edited

Loading

lydell commented May 28, 2022 •

edited

Loading

lydell commented May 28, 2022 •

edited

Loading

lydell commented May 29, 2022 •

edited

Loading

Janiczek commented May 29, 2022 •

edited

Loading