-
Notifications
You must be signed in to change notification settings - Fork 0
Add equalToFile to Expect
#1
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
Conversation
|
(Even if we do end up merging this, I have no idea how we would vendor it) |
|
I've opened a pull request on the main repo as well just hopefully to get some discussion going: elm-explorations#249 |
omnibs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
elm-test-rs says it also runs tests using node, so it might just work there.
juanedi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!! 🤘
brian-carroll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Looks useful!
I guess the main worries are
- compatibility with other runtimes (as you pointed out)
- blocking I/O
Other runtimes
We are helped here by the fact that all JS runtimes need Node compatibility! I think in practice the node: prefix convention can solve it for us.
The only alternative I can think of is to pass around a record of functions, like the "handle" pattern in Haskell, defining different constants for each supported runtime. The tricky part is that you couldn't do the requires or imports in the global scope. You'd have to do them dynamically, the first time they're needed, and then cache them for the next time.
But the node: prefix is easier, and should work in all practical cases. Might be good to test with elm-test-rs --deno
Blocking I/O
It's a pity that we need to use the sync versions of all the file operations, but there's no way out of it without rewriting everything. If users end up doing lots of snapshot testing, it could get slow, but at least it only blocks one worker thread and the others can make progress.
Actually it looks like elm-test-rs lets you specify the number of worker threads, so people could use that to increase the number of workers.
src/Elm/Kernel/Test.js
Outdated
| const fs = require('fs'); | ||
| const path = require('path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably worth adding the node: prefix to these Node built-ins.
Node supports this, and Deno and Bun both have Node-compatible APIs that require it.
It might be worth testing with elm-test-rs --deno (https://github.com/mpizenberg/elm-test-rs?tab=readme-ov-file#deno-runtime)
| const fs = require('fs'); | |
| const path = require('path'); | |
| const fs = require('node:fs'); | |
| const path = require('node:path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brian-carroll OK so I finally got around to looking into this and I was having trouble getting elm-test-rs --deno to run... have you ever done this successfully?
> elm-test-rs --deno ./tests/Page/Learn/GuidedDrafts/MainSpec.elm
Error: Deno supervisor failed to start
Caused by:
No such file or directory (os error 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't tried it when I made the comment.
I tried it just now and got the same message... and realised it meant I didn't have Deno installed! 😆
What do you get for which deno?
I installed it, tried again, and got an error that suggests elm-test-rs has not been updated to support more recent versions of Deno. Deno support seems to have been added 4 years ago. The current Deno version is 2.5.4. The latest v1 release is 1.46.3. elm-test-rs seems to have errors with both.
I still think importing from node:fs instead of just fs is the correct fix. It constitutes full support for both Deno and Bun as far as this repo is concerned, based on their own docs:
Deno's docs on Node compatibility
Bun's docs on Node compatibility
Beyond that, if users are still having issues, the problem is in another repo such as elm-test-rs.
❯ elm-test-rs --deno ./tests/Page/Learn/GuidedDrafts/MainSpec.elm
Warning `allow-hrtime` and `deny-hrtime` have been removed in Deno 2, as high resolution time is now always allowed
error: Uncaught (in promise) TypeError: Deno.read is not a function
const num = await Deno.read(rid, buf);
^
at _readTillDone (file:///Users/briancarroll/Documents/NoRedInk/monolith/ui/elm-stuff/tests-0.19.1/js/deno_linereader.mjs:6:28)
at Object.next (file:///Users/briancarroll/Documents/NoRedInk/monolith/ui/elm-stuff/tests-0.19.1/js/deno_linereader.mjs:25:38)
at file:///Users/briancarroll/Documents/NoRedInk/monolith/ui/elm-stuff/tests-0.19.1/js/deno_supervisor.mjs:108:16
71fef7c to
94a1f49
Compare
94a1f49 to
d7bac45
Compare
7b35077 to
d8fa6ea
Compare
|
@brian-carroll @juanedi @omnibs Hey friends, given that Juliano is getting close on his work to allow patching elm libraries via nix I've put some more effort into polishing this PR to get it ready for prime time:
Thanks to juliano, you can now easily test what this will feel like by checking out this PR and running |
src/Expect.elm
Outdated
|
|
||
| messageWithVisualDiff = | ||
| if String.endsWith ".html" filePath then | ||
| message ++ [ "To visually compare run: open file://" ++ existingAbsolutePath ++ " file://" ++ newAbsolutePath ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how hard we're trying to make this cross-platform but open is MacOS specific.
On Linux I think it's xdg-open. I don't know the command on Windows, but I think the path would have backslashes that would have to be converted to forward slashes for the URL.
I think it's best not to worry about this for now though! If this ever gets upstreamed it would need to be tested and patched by someone on those platforms.
f43cba3 to
dd21c67
Compare
|
Ok one last change before merging this into master! I've modified the failure case to create a new file like "filename.failure.html" right next to the existing one instead of dumping it to temp. This more closely aligns with how golden files work for spec tests and hopefully will have better visibility if the user loses track of the The write file call has been modified to try and delete the failure file if it exists - which also matches rspec behavior. |
juanedi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!! I do like Brian's suggestion :-)
|
Well done on this @micahhahn, nice addition! |
This PR adds a new primitive
equalToFileto theExpectmodule. It also adds aprettyPrintSinglemethodThis primitive allows us to write golden file / snapshot tests in Elm. These are particularly useful at capturing the current outputs of a program and then failing if the outputs change.
NoRedInk in particular has an interest in snapshotting the HTML at various points in elm-tests to a file. I've also added a
prettyPrintSinglefunction to the query module which exposes the internal query pretty print function. I think this will be useful generally for testing to be able to capture focus of a query in a complicated test (especially with elm-program-test). NRI would also see value in dumping the entire HTML of a page to a golden file so that we can do visual diffs of our pages in their various states.Note that the code its current state is very rough - the kernel code added presumes that the tests are being run in the context of a node environment. That's always true for node-test-runner but AFAIK not always true for elm-test-rs. I'm very open to any ideas that would make this more robust.
I'm also aware that doing the IO direclty inside of the
elm-explorations/testprobably prevents the test runners from doing proper watching on these golden files. Definitely open to ideas on approaches that would let the actual file IO be handled by the runner itself.Fixes HACKY-1182
Fixes LLM-2737
What does a failure look like?
Since the whole file diff would be an absolute mess (without running our diff algorithm to only show the minimal diff). So instead I write the changed file to temp so that we can run
git diffeasily. I also add a little touch of honey and if the file ends in *.html then we show a link to open the before and after in the browser.The git diff will look like this:
And just to show off how powerful this will be with something like Chrome's split view (chrome://flags/#side-by-side):
Screen.Recording.2025-10-27.at.2.21.09.PM.mov