Update url.app.apps.{get|upgrade} with required cluster arg. #1833
Conversation
That's fine. I would use the
I guess there should be an analogue way of getting the state? |
| import AppControls, { IAppControlsProps } from "./AppControls"; | ||
| import UpgradeButton from "./UpgradeButton"; | ||
|
|
||
| const mockStore = configureMockStore([thunk]); |
There was a problem hiding this comment.
I am a bit confused, the AppControls component is still a class, why all these changes are needed? Is it because of the change in react-redux?
There was a problem hiding this comment.
ah, you commented it in the PR:
it seems that even our class-based components are no longer classes after wrapping - at least, I had to remove calls to wrapper.state()
that's weird...
| expect(confirm.exists()).toBe(true); | ||
| confirm.props().onConfirm(); // Simulate confirmation | ||
|
|
||
| expect(wrapper.state("deleting")).toBe(true); |
There was a problem hiding this comment.
we should block this until we find a way to get the state? We are using the state in several tests, we would lose coverage. What's your plan here?
There was a problem hiding this comment.
Sorry, I forgot to include the info I found: Most people (myself included) recommend not testing state (ie. not testing implementation details) but instead validating what's rendered (the effect of the state. See this answer: https://stackoverflow.com/a/58432424 which I'd mostly agree with).
The quick and dirty solution on the same answer can actually be used by us (small tweak because we can't use shallow here, so don't have dive(). I had tried wrapper.find(".appControls").state() above which didn't work (same error), but what I didn't try, which does work, is explicitly wrapper.find(AppControls).state(). So I'm updating to that so we have a record of how we can do a quick-n-dirty change, but ultimately we should not be testing implementation details IMO.
There was a problem hiding this comment.
Generally I agree but sometimes is much faster to just test the state, is a trade-off.
absoludity
force-pushed
the
1762-pass-cluster-3
branch
from
0ba374a
to
7b2d47d
Compare
Continues url updates for cluster requirement. Ref #1762 .
Starting to get some duplication of test setup, as the function components which use state need providers etc. I looked at options for creating the shared code, but
react-scriptsdoesn't support the jestsetupFilesoption, so it might just be a separatesrc/helpers.test.tsxexporting some functions likegetWrapper(state, component, props)? What do you think? (not for this PR, but I will cleanup the dups once decided).The other thing that I had to wrangle here is that once wrapped in a provider, it seems that even our class-based components are no longer classes after wrapping - at least, I had to remove calls to
wrapper.state()(also triedwrapper.find(ComponentName).state()) as jest kept failing saying thatstate()is only available on class-based components.