avh4 / elm-program-test

Test Elm programs
https://elm-program-test.netlify.com/
MIT License
93 stars 27 forks source link

Upgrade to elm-explorations/test 2.0.0 #173

Closed Janiczek closed 1 year ago

Janiczek commented 1 year ago

Screenshot 2022-10-12 at 13 32 41


This had to deal with the removal of the deprecated (for 5 years now) function Test.Runner.Failure.format. I've opted for the approach elm-test-runner chose: vendor an implementation of that function from node-test-runner.

avh4 commented 1 year ago

Thanks! I'll be back from vacation by the end of the week.

stoeffel commented 1 year ago

Thanks! I'll be back from vacation by the end of the week.

enjoy your vacation 🏖️

avh4 commented 1 year ago

This seems like a lot of code to copy in, which seems to be duplicating all of the error printing logic in elm-test? Do you know why it was deprecated? And is there a plan for something to replace it? In my case, I really don't want to call into that code; I just want to be able to add some additional information to the error report.

Also, do you know why Vendored.Diff is needed instead of using the public jinjor/elm-diff ?

Janiczek commented 1 year ago

Do you know why it was deprecated?

No idea, sorry. I couldn't find any discussion around it in the issue/PR history of the repo. The deprecation notice just appeared ~5y ago.

And is there a plan for something to replace it?

Not currently. I suppose elm-test-runner or some similar package could expose it. (cc @mpizenberg)

Also, do you know why Vendored.Diff is needed instead of using the public jinjor/elm-diff?

This is a question for the runner codebases (and again it probably dates beyond the current maintainers), but I assume they wanted to minimize the number of dependencies they have, so they vendored it.

mpizenberg commented 1 year ago

I don’t know why Test.Runner.Failure.format was deprecated. Regarding all the vendoring in elm-test and elm-test-runner (the elm package for elm-test-rs), this is because the elm language is super strict regarding having only 1 version of a given package installed and usable for a given compilation. So we want tooling dependencies as broad as possible. If we use a community package, and that package is updated at some point, users of that package will no longer be able to use elm-test or elm-test-rs unless these also update to the newest version. Other languages like JS, Rust and others are not as strict, and allow multiple versions being present at compilation (with some rules) as long as they don’t interact with each other in incompatible ways. I did an exception with elm-test-rs, where I’m using my mpizenberg/elm-test-runner package where all the elm stuff of the runner is living, but it’s clearly stated at the beginning of the README that this package should not be used by people outside of elm-test-rs context.

mpizenberg commented 1 year ago

I suppose elm-test-runner or some similar package could expose it. (cc @mpizenberg)

Depending on mpizenberg/elm-test-runner would not be wise since this package is often bumped when improvements to elm-test-rs are done. It’s currently already at version 6.0.0. So depending on it in elm-program-test will very likely endup with new versions of elm-test-rs being incompatible with elm-program-test.

avh4 commented 1 year ago

Ah, okay, I'll merge this for now.

For future elm-test, I'd like to have some API for building error messages for expectation helpers. I think the main thing needed would be something like addFailureContext : String -> Expectation -> Expectation that would prepend the string to the failure message (unlike onFail that replaces the entire message). Though probably a bit more though into it would be good (maybe it should have a separate param to give the name of the helper function as well as the extra details, or some optional params for certain types of info, or have some otherwise more structured data type for information about failures). This would I guess also require a major version since it would I guess need to add more variants to the Reason type.

Is anyone going to be around actively to help review an addition like that?

Janiczek commented 1 year ago

Is anyone going to be around actively to help review an addition like that?

I plan to be around for reviews đź‘‹

stoeffel commented 1 year ago

Is anything blocking this PR from merging? @avh4

ghost commented 1 year ago

Is anyone going to be around actively to help review an addition like that?

I plan to be around for reviews wave

Hi @Janiczek , would you mind publishing your fork in the meantime? Thanks!