test(profiling): Add test utils to validate Profile Chunk envelope #18170
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 'fibonacci', | ||
| ], | ||
| // Should be at least 20ms based on our setTimeout(21) in the test | ||
| minSampleDurationMs: 20, |
There was a problem hiding this comment.
Bug: Reduced duration validation allows regressions.
The duration validation changed from 200ms to 20ms during refactoring. The original test checked expect(durationSec).toBeGreaterThan(0.2) where durationSec was in seconds (0.2s = 200ms), but the refactored version passes minSampleDurationMs: 20 (20ms), making the test 10x less strict and potentially allowing performance regressions to pass.
There was a problem hiding this comment.
It should actually be 20, so it was wrong before
| 'largeSum', | ||
| ], | ||
| // Test that profile duration makes sense (should be > 20ms based on test setup | ||
| minSampleDurationMs: 20, |
There was a problem hiding this comment.
Bug: Compromised performance test allows regressions.
The duration validation changed from 200ms to 20ms during refactoring. The original test checked expect(durationSec).toBeGreaterThan(0.2) where durationSec was in seconds (0.2s = 200ms), but the refactored version passes minSampleDurationMs: 20 (20ms), making the test 10x less strict and potentially allowing performance regressions to pass.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ], | ||
| // Should be at least 20ms based on our setTimeout(21) in the test | ||
| minSampleDurationMs: 20, | ||
| isChunkFormat: true, | ||
| }); |
There was a problem hiding this comment.
Bug: minSampleDurationMs for chunk format tests is incorrectly set to 20ms, a 10x reduction from 200ms.
Severity: MEDIUM | Confidence: 1.00
🔍 Detailed Analysis
The minSampleDurationMs threshold for chunk format tests has been incorrectly reduced from 200ms to 20ms. Previously, expect(durationSec).toBeGreaterThan(0.2) (0.2 seconds) was used, but the new minSampleDurationMs: 20 value, when compared against durationMs (converted from seconds), sets the threshold to 20ms. This 10x reduction significantly weakens the test's ability to detect issues with profile duration capture. The accompanying comment "Should be at least 20ms based on our setTimeout(21) in the test" is misleading for these tests.
💡 Suggested Fix
Set minSampleDurationMs to 200 for chunk format tests to restore the original 200ms duration threshold.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
dev-packages/browser-integration-tests/suites/profiling/traceLifecycleMode_multiple-chunks/test.ts#L68-L72
Potential issue: The `minSampleDurationMs` threshold for chunk format tests has been
incorrectly reduced from 200ms to 20ms. Previously,
`expect(durationSec).toBeGreaterThan(0.2)` (0.2 seconds) was used, but the new
`minSampleDurationMs: 20` value, when compared against `durationMs` (converted from
seconds), sets the threshold to 20ms. This 10x reduction significantly weakens the
test's ability to detect issues with profile duration capture. The accompanying comment
"Should be at least 20ms based on our setTimeout(21) in the test" is misleading for
these tests.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
@s1gr1d I think was unintentionally set to 200ms before, what is the expected testcase?
There was a problem hiding this comment.
Yeah it should be 20 actually :D
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I'm not the biggest fan of these extracted assertion helpers. Mainly because these helpers usually become increasingly "versatile" (read "complex") and ending up with branches in assertions is always a bit hard to grasp. I personally take a bit of repetition over it.
That being said, I can see the value here so feel free to merge anyway.
This PR adds two utility functions for testing the profile envelope:
validateProfilePayloadMetadataandvalidateProfile. As More tests are going to be added, I don't want to copy-paste the same tests over and over.Part of #17279