Skip to content

[FSSDK-9152] Fix "client_initialized" event not firing #820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 3, 2023

Conversation

mikechu-optimizely
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely commented May 2, 2023

Summary

  • Fix "client_initialized" event not firing

Test plan

  • Needs a more detailed set of tests wrapped around for coverage.
  • Existing tests should continue to succeed along with FSC

Issues

FSSDK-9152

@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review May 2, 2023 21:48
@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner May 2, 2023 21:48
assert.equal(readyData.success, true);
assert.isUndefined(readyData.reason);

setTimeout(() => sinon.assert.callCount(apiManager.sendEvents, 1), 100);
Copy link
Contributor Author

@mikechu-optimizely mikechu-optimizely May 2, 2023

Choose a reason for hiding this comment

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

This really needs to check sendEvents(events) has a "client_initialized" in the array.

This ancient version of sinon is really hard to work with.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we have a separate unit test for "registerVuid" which validates "client_initialized" event. If so, this may be enough to validate we have one event generated here.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

assert.equal(readyData.success, true);
assert.isUndefined(readyData.reason);

setTimeout(() => sinon.assert.callCount(apiManager.sendEvents, 1), 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we have a separate unit test for "registerVuid" which validates "client_initialized" event. If so, this may be enough to validate we have one event generated here.

@coveralls
Copy link

Coverage Status

Coverage: 90.536%. Remained the same when pulling 870b5ce on mike/client_initialized-fix into 2580ea0 on master.

@mikechu-optimizely
Copy link
Contributor Author

@opti-jnguyen I found the test that was failing (doing its job dutifully despite me ignoring it). I'm good to go especially with @jaeopt 's approval.

Copy link
Contributor

@opti-jnguyen opti-jnguyen left a comment

Choose a reason for hiding this comment

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

LGTM overall. Agree with your note on the test, you may be able to grab the args from the spy and do an assert equal.

Maybe something along the lines of:

const sendEventArgs1 = fakeEventManager.sendEvent.args;
assert.deepEqual(
  sendEventArgs1[0].toString(),
  new OdpEvent('fullstack', 'client_initialized', new Map([['fs_user_id', 'fsUserA']]), new Map()).toString()
);

That being said, if that doesn't work I'm down to merge and address later.

@opti-jnguyen opti-jnguyen merged commit 34a7bf0 into master May 3, 2023
@opti-jnguyen opti-jnguyen deleted the mike/client_initialized-fix branch May 3, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants