Skip to content

Conversation

@mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Jun 2, 2020

Summary

  • Several unit tests weren't properly stubbing the default event dispatcher - add these stubs
  • The test suite wasn't exiting due to active timers not being cleaned up. Fixed this by mocking setTimeout using sinon.useFakeTimers
  • One test for Node default event dispatcher was relying on a real connection error from attempting to request to an invalid domain. Added a nock mock for this to prevent the real request.

Test plan

Run npm test. Should exit immediately, and not make any outgoing HTTP requests.

Issues

#487

@mjc1283 mjc1283 requested a review from a team as a code owner June 2, 2020 00:13
@mjc1283 mjc1283 removed their assignment Jun 2, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 96.7% when pulling d09fdb7 on mcarroll/fix-hanging-tests into 95cd8a0 on master.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines -169 to -181
it('should allow passing of "react-sdk" as the clientEngine', function() {
var optlyInstance = optimizelyFactory.createInstance({
clientEngine: 'react-sdk',
datafile: {},
errorHandler: fakeErrorHandler,
eventDispatcher: fakeEventDispatcher,
logger: silentLogger,
});
// Invalid datafile causes onReady Promise rejection - catch this error
optlyInstance.onReady().catch(function() {});
assert.equal('react-sdk', optlyInstance.clientEngine);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this because it's identical to the test above.

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.

4 participants