Conversation
…e" instead of instantiating Time() directly.
…ovider instead of Thread.sleep(), making tests deterministic and ~25 seconds faster. - Adds StubSDKCore.advanceTimeBy() method to advance simulated time, and updates all test methods to use explicit time advancement rather than actual sleeping. - Updates StopResourceWithError in DatadogRumMonitor to use getEventTime() so it respects the controllable timeProvider.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3154 +/- ##
===========================================
- Coverage 70.94% 70.92% -0.02%
===========================================
Files 912 912
Lines 33525 33528 +3
Branches 5635 5635
===========================================
- Hits 23784 23779 -5
- Misses 8166 8172 +6
- Partials 1575 1577 +2
🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 230ad6c0e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val eventTime = getEventTime(attributes) | ||
| handleEvent( | ||
| RumRawEvent.StopResourceWithError( | ||
| key, |
There was a problem hiding this comment.
Apply eventTime to ResourceId error stops consistently
Only the String-key stopResourceWithError now uses getEventTime(attributes); the ResourceId overload later in this file still constructs RumRawEvent.StopResourceWithError without an explicit eventTime, so _dd.timestamp and controllable time providers are ignored for ResourceId-based resource errors (e.g., network instrumentation). When tests advance time via StubSDKCore or when a non-default time provider is used, error events can end up on wall-clock while start/stop resource events use the controlled clock, leading to inconsistent ordering or durations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good observation that should probably be fixed. However, this method is most likely not used from cross-platform SDKs, so it is probably fine.
There was a problem hiding this comment.
We have to fix it for both ResourceId option and legacy string key option as well.
There was a problem hiding this comment.
Ticket created. I’ll fix it in a separate PR. Thanks to both of you. 🙂
What does this PR do?
This PR refactors
ViewLoadingTimeMetricsTeststo use a controllable time provider instead ofThread.sleep(). It adds anadvanceTimeBy()method toStubSDKCorethat allows tests to control time advancement, making tests deterministic and significantly faster (~25 seconds saved per test run). All test methods are updated to use explicit time advancement, andDatadogRumMonitor.stopResourceWithError()now usesgetEventTime()to respect the controllable time provider.Motivation
ViewLoadingTimeMetricsTestscontained the same flaky test multiple times, and I believe some of the flakiness was due to how we were managing time withinDatadogRumMonitor. We were instantiating Time directly inside the monitor instead of using the values provided by a time provider. With that approach, tests were able to modify time values, which made them non-deterministic because they depended on real time.With the proposed changes, we can fully control time values and switch to deterministic tests.
Additional Notes
I plan to open a separate PR to remove the default value for eventTime in
RumRawEventso that we use the timeProvider values everywhere.For example:
Any reasons not do it?
Review checklist (to be filled by reviewers)