-
Notifications
You must be signed in to change notification settings - Fork 49.6k
Fix some DevTools regression test actions and assertions #34459
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
Fix some DevTools regression test actions and assertions #34459
Conversation
Comparing: 47664de...f4e1b8c Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
// @reactVersion >= 16.6 | ||
it('should filter Suspense', async () => { |
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.
<Suspense>
was renamed from unstable_Placeholder
in 16.6
if ( | ||
semver.gte(ReactVersionTestingAgainst, '15.0.0') && | ||
semver.lt(ReactVersionTestingAgainst, '17.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 don't trust how SemVer comparisons works with 0.0.0-experimental
ranges so I flipped these branches to only use classic runtime for this particular range. The previousl else
branch silently flipped to modern runtime when the defaults changed in Babel.
57847e4
to
1aa91f3
Compare
: existingNameElements[0].innerText | ||
// remove trailing colon | ||
.slice(0, -1); |
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.
Accounts for https://github.com/facebook/react/pull/34095/files#diff-94a124a1b825898267d7c9f43ef03cafaf2f55da968b1bd46d1f5ffea9def7bfR233.
Didn't think it's worth it to add even more test-specific markup
const isLegacySuspense = React.version.startsWith('17'); | ||
if (isLegacySuspense) { | ||
expect(commitData[0].fiberActualDurations).toMatchInlineSnapshot(` | ||
Map { | ||
1 => 15, | ||
2 => 15, | ||
3 => 5, | ||
4 => 3, | ||
5 => 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.
Need to bisect where the additional node comes from. I suspect something legacy Suspense related similar to the other snapshot tests that had a <Lazy>
show up.
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.
`, | ||
` | ||
[root] | ||
"[root] |
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.
Where do these string brackets come from? Can this be solved via formatting? Because we don't append them in other test cases
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 had to switch to toEqual
instead of using toMatchInlineSnapshot
to be able to snapshot without the Suspense tree.
d332d6a
to
f4e1b8c
Compare
Stacked on #34456
I wanted to make sure the recent Suspense tree in the backend didn't break older React versions. This revealed one reorder bug for normal elements that broke in c97ec75 which I'm attempting to fix in #34464
preprocessData
andtimelineProfiler
tests are still failing. Both tests were codemodded to modernscheduler/unstable_mock
APIs that we need to add a compat layer for but I won't do that here. Probably as a follow-up once the rest is green.