-
Notifications
You must be signed in to change notification settings - Fork 41
Support providing a fake Browser.Navigation.Key in tests #243
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
base: master
Are you sure you want to change the base?
Conversation
], | ||
"elm-version": "0.19.0 <= v < 0.20.0", | ||
"dependencies": { | ||
"elm/browser": "1.0.2 <= v < 2.0.0", |
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 wonder whether this is as low as we can go, or whether 1.0.1 or 1.0.2 have something important we want to encourage adoption of.
(We did that with bytes 1.0.8 I think, where there was something substantial... but I can't remember the details)
src/Test.elm
Outdated
Internal.ElmTestVariant__Labeled desc (Internal.ElmTestVariant__UnitTest (\() -> [ thunk () ])) | ||
|
||
|
||
{-| Return a [`Test`](#Test) that requires a [`Browser.Navigation.Key`](https://package.elm-lang.org/packages/elm/browser/latest/Browser-Navigation#Key) |
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.
"requires" was surprising to me. I'd say it provides a key instead?
I'm wondering whether it would be benefitial to also mention elm-program-test or Browser.application init
functions. They're not the only scenario an user might need the Key for, but might be the most common one?
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.
A third note: Is there a gap in the API now where Test.testWith
users still can't get to the key? This might be getting into a "we need to redesign testWith
to be a builder pattern instead" territory, IDK yet... 🤔
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've improved the wording a bit, let me know what you think. (Don't know why I used "require" 🤷♂️ )
I've mentioned Browser.application, but I've purposefully ignored elm-program-test
because it is not a core library. Someone could fork that project or make something similar but better one day and this package would push towards one solution rather than another, which I don't think is fair.
And yes, I think there's a gap in the API. It definitely reeks of multiple combinations of options, so a builder-like pattern is starting to feel like it could be justified. I don't think we're there yet but your mileage may vary. If there was one more kind of option then I definitely think there would have to be some re-design.
So should we add a new function? and name it... testWithWithKey
? 🤢
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.
If a builder pattern is possible, I'd definitely prefer it to testWith
(where you suddenly have to care about distributions even if you just want to up the run count).
If we can do that without a final |> build
, that would be (roughly) 100000x better.
I can try and brainstorm something on another branch.
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.
Just to clarify, when you said Test.testWith
, did you mean Test.fuzzWith
?
0d3059d
to
5a40402
Compare
Adds
Test.testWithKey
andTest.fuzzWithKey
which provide aBrowser.Navigation.Key
to tests.These functions are designed in a way that the key can't be used in production code in a way that will have a noticeable effect (one could generate a test in production code, but they can't really do anything with a test).
Currently, the workaround is to define a wrapper type
as well as wrapper functions for
Browser.Navigation
functions (such aspushUrl
) that accept this type, where they'll do nothing if the key is not a real key.Implemention-wise, the key is a simple function with a side-effect and no input (source).
I've introduced a new test key which is a plain noop function, which should be equivalent in all "noticeable" ways.
Releasing this functionality will lock the implementation of
elm/browser
's key to such a function, at least until it releases a new major version.