Ignore ProfileChunk entries that have no useful data#4485
Conversation
| expect(profile).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('successfully imports a chrome profile with an invalid "endTime" entry', async () => { |
There was a problem hiding this comment.
I chcked that this test was crashing on the main branch.
| const text = buffer.toString('utf8'); | ||
| const buffer = zlib.gunzipSync(compressedBuffer); | ||
| const events: TracingEventUnion[] = JSON.parse(buffer.toString('utf8')); | ||
| events.push({ |
There was a problem hiding this comment.
this data comes directly from one of the failing profiles.
julienw
force-pushed
the
fix-chrome-importer
branch
from
a81ab8a to
602fcf7
Compare
| }); | ||
|
|
||
| it('successfully imports a non-chunked profile (one that uses a CpuProfile trace event)', async function () { | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
Note: I removed the Image.addEventListener mock for this test because this is used for the screenshots, and the unchunked profile doesn't have any.
| data.category === 'Navigation' && | ||
| data.innerWindowID | ||
| ) { | ||
| const innerWindowID = data.innerWindowID; |
There was a problem hiding this comment.
Flow was failing with innerWindowID is missing in ChromeDurationTraceEventPayload` and this is the only way I could silence it.
| export type ChromeDurationTraceEventPayload = {| | ||
| type: 'tracing', | ||
| category: 'FromChrome', | ||
| category: string, |
There was a problem hiding this comment.
For some reason this started to error after I added FallbackEndEvent 🤔 Indeed there's a place where we pass an arbitrary string...
There was a problem hiding this comment.
Oh interesting. Good to put it as string then if we have a place where we pass an arbitrary string.
Codecov ReportBase: 88.64% // Head: 88.64% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4485 +/- ##
=======================================
Coverage 88.64% 88.64%
=======================================
Files 284 284
Lines 25591 25588 -3
Branches 6885 6883 -2
=======================================
- Hits 22684 22682 -2
+ Misses 2702 2701 -1
Partials 205 205
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for fixing this annoying issue!
| const text = buffer.toString('utf8'); | ||
| const buffer = zlib.gunzipSync(compressedBuffer); | ||
| const events: TracingEventUnion[] = JSON.parse(buffer.toString('utf8')); | ||
| events.push({ |
| export type ChromeDurationTraceEventPayload = {| | ||
| type: 'tracing', | ||
| category: 'FromChrome', | ||
| category: string, |
There was a problem hiding this comment.
Oh interesting. Good to put it as string then if we have a place where we pass an arbitrary string.
|
Thanks for the review! |
2 participants
Fixes #4308
You'll probably surprised by some of the changes in the first commit: indeed when I changed the types for the Chrome importer, this produced a chain of Flow errors that I tried to fix with the simplest changes. Hopefully that will be OK.
Then I was surprised that these type changes didn't show the problem afterwards: Flow wouldn't error on this line.
I didn't investigate more as I wanted to fix the issue and didn't want to spend more time than necessary on this.
You can test with the profile from #4308 or alternatively the profile from bugzilla bug 1817623.
production / deploy preview