Skip to content

Conversation

@jaissica12
Copy link
Contributor

Background

  • Evaluating Setup and Teardowns for each test file

What Has Changed

  • Updated test files:
    • test/src/tests-identities-attributes.ts
    • test/src/tests-identity.ts
    • test/src/tests-identityApiClient.ts

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 marked this pull request as ready for review October 20, 2025 21:22
@jaissica12 jaissica12 requested a review from rmi22186 October 21, 2025 15:48
captureConsole,
mocha: {
timeout: 5000 // 5 seconds. Increase from default 2 seconds.
timeout: 10000 // 10 seconds. Increase from default 2 seconds.
Copy link
Member

Choose a reason for hiding this comment

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

Strange, still seeing a 2000 ms issue here for firefox

Image

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, may be the comment i left above actually in the utils.js file for waitForCondition instead

waitForCondition = function async(
conditionFn,
timeout = 200,
timeout = 2000,
Copy link
Member

Choose a reason for hiding this comment

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

i realize i commented below seeing that there was still a 2000ms timeout for firefox, but i realize maybe that's here instead

Image

Comment on lines 1056 to 1057
await waitForCondition(hasIdentityCallInflightReturned);
await waitForCondition(() => {
Copy link
Member

Choose a reason for hiding this comment

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

this should be either the await you added OR the await that's already here. there isn't really a reason to have both. I'd test again using just the bottom one, because the fetchMock on line 1051 i the reason the current waitForConfition should be working. if the one you added is more reliable though, that's fine

Comment on lines 1249 to 1255
await waitForCondition(() => {
const currentUser = mParticle.Identity.getCurrentUser();
const userIdentities = currentUser?.getUserIdentities()?.userIdentities;
return userIdentities &&
userIdentities.customerid === 'customerid1' &&
userIdentities.email === '[email protected]';
});
Copy link
Member

Choose a reason for hiding this comment

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

I think I see why the waitForCondition for identityCallInFlight === false may not be reliable. because it is set to false BEFORE parsing the identityResponse, which is where a lot of the additional logic about making a new current user, happens. So we can probably remove the waitForCondition and leave yours.

Copy link
Member

Choose a reason for hiding this comment

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

This can be a future cleanup ticket once we merge everything in, but at the bottom of the parseIdentityResponse, we do log Successfully parsed Identity Response. I wonder if we should be mocking the logger and waiting for this to be called and then this will basically allow us to know that an identity has been fully returned AND completely parsed.

mParticle.getInstance()._Store.identityCallInFlight === false
);
});
// switching back to logged in user should not result in any UIC events
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 keep this comment

mParticle.init(apiKey, window.mParticle.config);

await waitForCondition(hasIdentityCallInflightReturned)
await waitForCondition(hasIdentityCallInflightReturned);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await waitForCondition(hasIdentityCallInflightReturned);
await waitForCondition(hasIdentityCallInflightReturned);

@jaissica12 jaissica12 requested a review from rmi22186 October 28, 2025 15:49
hasIdentityResponseParsed = (loggerSpy) => {
return () => loggerSpy?.verbose?.getCalls()?.some(call =>
call.args[0] === 'Successfully parsed Identity Response'
) || false;
Copy link
Member

Choose a reason for hiding this comment

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

the || false here is unnecessary since the .some() already returns a boolean

Suggested change
) || false;
);


fetchMock.resetHistory();
// Clear out before each init call
await waitForCondition(hasBeforeEachCallbackReturned);
Copy link
Member

Choose a reason for hiding this comment

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

If you add await waitForCondition anywhere in this file, I'd suggest adding it above resetForTests in the same codeblock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally makes sense since we're checking if the beforeEach init has completed before resetting. Updated for all occurrences in the code.

Comment on lines 1819 to 1820
await waitForCondition(hasIdentityCallInflightReturned);
await waitForCondition(hasLoginReturned);
Copy link
Member

Choose a reason for hiding this comment

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

i think this can be just

Suggested change
await waitForCondition(hasIdentityCallInflightReturned);
await waitForCondition(hasLoginReturned);
await waitForCondition(hasIdentityResponseParsed(loggerSpy));

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, missed that. Updated the test now.

@jaissica12 jaissica12 requested a review from rmi22186 October 28, 2025 20:28
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

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