Skip to content

Conversation

@jaissica12
Copy link
Contributor

Background

  • This PR is culmination of all the PRs evaluating test setup for each file.

What Has Changed

  • Updated test setup for each file.

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

  • {Any additional information or context relevant to this PR}

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

@jaissica12 jaissica12 changed the title Test/sdke 303 evaluate each integration test setup test: SDKE 303 evaluate each integration test setup Oct 28, 2025
@jaissica12 jaissica12 requested a review from rmi22186 October 30, 2025 20:19
@jaissica12 jaissica12 marked this pull request as ready for review October 30, 2025 20:19
@jaissica12 jaissica12 force-pushed the test/SDKE-303-evaluate-each-integration-test-setup branch from 7a3cc3d to 981133a Compare October 30, 2025 21:05

// Dispatching event will trigger upload process
window.dispatchEvent(new Event('pagehide'));
await Promise.resolve();
Copy link
Member

Choose a reason for hiding this comment

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

I see a few instances of await Promise.resolve() still, are these necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I think I missed these in the test-specific PRs. I’ve now removed all occurrences of await Promise.resolve() from both the beaconUpload and batchUploader tests, as there were a few instances which were not needed in that test file as well.

expect(JSON.parse(window.sessionStorage.getItem(eventStorageKey)).length, 'Events should be populated before dispatch').to.equal(3);
expect(uploader.batchesQueuedForProcessing.length, 'Batch Queue should be populated before dispatch').to.equal(3);

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.

I see line 64 currently in master
https://github.com/mParticle/mparticle-web-sdk/blob/master/test/src/tests-beaconUpload.ts#L77
but I don't see line 137 below. what happens if you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yesterday i was running each test in isolation to identify which ones were failing, and this test was failing at that time, which is why I added it. However, I don’t think it’s needed, it can be safely removed without affecting the test suite, since setting window.onbeforeunload = null in this test has no impact on the SDK handler.

@jaissica12 jaissica12 requested a review from rmi22186 October 31, 2025 14:29
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

@jaissica12 jaissica12 merged commit c6faff3 into test/SDKE-302-evaluate-resetForTests-for-flakey-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