[react-testing] Disable flakly test for React 17 #2435
Conversation
melnikov-s
force-pushed
the
disable-stale-promises-test-for-react-17
branch
from
f5de3be to
356b4e5
Compare
| wrapper.act(() => new Promise(() => {})); | ||
| await wrapper.destroy(); | ||
| // eslint-disable-next-line no-process-env | ||
| if (process.env.REACT_VERSION !== '17') { |
There was a problem hiding this comment.
This seem reasonable although I don't know if anyone is aware of a better way of achieving this as I didn't see any precent of the use of process.env.REACT_VERSION in the codebase.
There was a problem hiding this comment.
Oh never mind, I just realized we're in the quilt repot and not in web so I do see a couple uses of REACT_VERSION. Still, not too sure if this the best way so I would let someone more familiar with quilt chime in on this and I can give a +1 if need be.
There was a problem hiding this comment.
Rando idea to avoid nesting adjustments:
const itExceptReact17 = process.env.REACT_VERSION !== '17' ? it : it.skip;
itExceptReact17('releases any stale promises when component is destroyed', async () => {If the eventual plan is to remove the "stale promise releasing" behaviour in 17, and make it a react 18 only feature, as the behaviour in v17 is not stable for unknown reasons then I think I'd prefer it if we roll this change and that disabling in the same PR rather than have two separate PRs.
There was a problem hiding this comment.
Agreed, better to still have the it block but have it be skipped (shows up in test reporters that way)
Alternative approach (either is fine with me, I like this idea from the original feature request):
const itIf = condition => condition ? it : it.skip;
itIf(process.env.REACT_VERSION !== '17')('blah blah', () => {
...
});There was a problem hiding this comment.
I'm going to commit this change and push it up so that we can get this merged because we need it to unblock a version release
There was a problem hiding this comment.
Sounds good @ryanwilsonperkin , this PR was mostly meant to unblock tests while I work a fix for 17 queue emptying or disabling it all together. I had sometime this morning and I might have found a fix that I'll put up later.
| @@ -0,0 +1,2 @@ | |||
| --- | |||
There was a problem hiding this comment.
This is a changeset file, it's a new bit of tooling that we've adopted in the past couple of months that helps automatically generate changelog entries and version bumps.
| @@ -173,7 +175,8 @@ describe('@shopify/react-testing', () => { | |||
| ); | |||
| } | |||
|
|
|||
| it('releases any stale promises when component is destroyed', async () => { | |||
| // eslint-disable-next-line no-process-env | |||
| itIf(process.env.REACT_VERSION !== '17')('releases any stale promises when component is destroyed', async () => { | |||
There was a problem hiding this comment.
I discovered that React exports a version variable that contains the what the name suggests - https://unpkg.com/browse/react@17.0.2/cjs/react.development.js#L2331
Perhaps we could use that? Something like:
const isReact18OrMore = parseInt(version.split('.')[0], 10) >= 18;
// ...
itIf(isReact18OrMore)(...
|
I think we can close this now as #2438 supersedes it |
ryanwilsonperkin
force-pushed
the
disable-stale-promises-test-for-react-17
branch
from
48e4887 to
8520cd1
Compare
Description
Fixes (issue #)
Prevents a flakly tests from running in React 17.
Related PR: #2352