Skip to content

Conversation

@jaissica12
Copy link
Contributor

@jaissica12 jaissica12 commented Oct 14, 2025

Background

  • This PR is an effort to reduce flaky tests due to timing variance, and session timeouts.

What Has Changed

  • Updated resetForTests function for:
    • Test-only defaults (isTestEnv): eventBatchingIntervalMillis, astBackgroundEvents, offlineStorage (and cleared window.onbeforeunload).
    • Safe teardown: cleared per-instance session globalTimer and global window.mParticle.forwardingStatsTimer.

Screenshots/Video

  • {Include any screenshots or video demonstrating the new feature or fix, if applicable}

Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have tested this locally.

Additional Notes

  • To keep scope of this PR limited this just updates resetForTests function.
  • A follow up ticket SDKE-303 to this will update each test to clear timers/intervals.

Reference Issue (For employees only. Ignore if you are an outside contributor)

@jaissica12 jaissica12 force-pushed the test/SDKE-302-evaluate-resetForTests-for-flakey-tests branch from d600e65 to 0d87d77 Compare October 14, 2025 15:32
@jaissica12 jaissica12 changed the base branch from test/SDKE-300-convert-all-then-to-async-await-in-tests to test/SDKE-301-resolve-failing-integration-tests October 14, 2025 15:32
@jaissica12 jaissica12 marked this pull request as ready for review October 14, 2025 20:16
@jaissica12 jaissica12 requested a review from rmi22186 October 14, 2025 20:16
};

this._resetForTests = function(config, keepPersistence, instance) {
if (typeof window !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this additional resetForTests functionality into the config/setup in the test folder to avoid increasing the size of the SDK for test purposes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since I realize we use ._resetForTests all over the place, I'd expect it to be something like:

const originalRFT = mParticle._resetForTests;

mParticle._resetForTests = function() {
  // all this code here
  originalRFT();
}

This way all the future instances of ._resetForTests will run the code + the original _resetForTests function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added additional code in setup.ts to keep everything together. I believe we're already resetting flags there and calling _resetForTests, so the additional code can be added here. Let me know thoughts

@jaissica12 jaissica12 requested a review from rmi22186 October 23, 2025 21:05
const forwardingStatsTimer = mParticleSDK?._forwardingStatsTimer;
if (typeof forwardingStatsTimer === 'number' && mParticleSDK) {
clearInterval(forwardingStatsTimer);
mParticleSDK._forwardingStatsTimer = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason this is null but line 230 is 0? I think we should set both to 0

};


window.onbeforeunload = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for setting this to null? seems like we should be setting it/mocking it for specific tests that require onbeforeunload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, individual tests like tests-beaconUpload.ts also set it to null before dispatching events, but having it in the global setup acts as a safeguard that ensures clean test isolation for both current and future tests.
When I started working on this I was exploring potential causes of flakiness, so this is just defensive approach even if a someone forgets to clear onbeforeunload in a specific test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think we should have it only in the test file it's being used. There's a chance an integration test depends on onbeforeunload in the future, and I'd rather be explicit about when it should be null and when it shouldn't. This can probably be part of a cleanup task.

@jaissica12 jaissica12 requested a review from rmi22186 October 28, 2025 15:23
@jaissica12 jaissica12 changed the base branch from test/SDKE-301-resolve-failing-integration-tests to test/SDKE-300-convert-all-then-to-async-await-in-tests October 30, 2025 20:22
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
9.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@jaissica12 jaissica12 requested a review from rmi22186 October 31, 2025 18:26
@jaissica12 jaissica12 merged commit 302dd47 into test/SDKE-300-convert-all-then-to-async-await-in-tests Oct 31, 2025
24 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants