refactor: random test failures refactor #988
Conversation
Codecov Report
@@ Coverage Diff @@
## master #988 +/- ##
==========================================
- Coverage 80.77% 79.78% -0.99%
==========================================
Files 86 86
Lines 4384 4383 -1
Branches 793 793
==========================================
- Hits 3541 3497 -44
- Misses 584 628 +44
+ Partials 259 258 -1
Continue to review full report at Codecov.
|
test/unit/instance-lookup-test.js
Outdated
| new InstanceLookup().instanceLookup(options, () => {}); | ||
| assert.ok(spy.called, 'Failed to call dns.lookup on hostname'); | ||
| assert.ok(spy.calledWithMatch(options.server), 'Unexpected hostname passed to dns.lookup'); | ||
| done(); |
There was a problem hiding this comment.
It looks like done is no longer called asynchronously, so it'd be better to make the tests synchronous by removing done from the test function completely. What do you think? 🙇
There was a problem hiding this comment.
Sure thing.
By the way, there were some random failures happen within sender tests as well. I have reviewed them, and I have not identified any significant differences with the previous version. The only thing I changed is moving one function to its corresponding test.
I will paste the error logs here, see if you can share some insight on this. Thanks! 🙇
- Sender send to hostnam
[00:01:14] Send
[00:01:14] "after each" hook:
[00:01:14] Uncaught AssertionError: Unexpected hostname passed to dns.lookup: expected false to be truthy
[00:01:14] at ok (test\unit/instance-lookup-test.js:335:14)
[00:01:14] at callback (src/instance-lookup.ts:59:13)
[00:01:14] at GetAddrInfoReqWrap.cb [as callback] (src/sender.ts:138:16)
[00:01:14] at GetAddrInfoReqWrap.onlookupall [as oncomplete] (dns.js:79:17)
[00:01:14]
[00:01:14]
[00:01:14] 3) Sender send to hostnam
[00:01:14] Send
[00:01:14] "before each" hook in "Send":
[00:01:14] TypeError: Cannot read property 'call' of undefined
[00:01:14]
[00:01:14]
[00:01:14] 4) Parallel Send Strategy
[00:01:14] Send
[00:01:14] "after each" hook for "should send all IPs success":
[00:01:14] TypeError: Cannot read property 'testSockets' of undefined
[00:01:14] at Context.testSockets (test\unit/sender-test.js:329:30)
There was a problem hiding this comment.
Could these sender test failures also because there are done() function called during the process which is causing conflicts?
|
Only remove the done callbacks from the two tests that I modified for instance-lookup since these two tests are not async anymore. The done callbacks for other tests are left as it is. However, the tests will still pass if we remove them. The done() suppose to prevent the tests to be finished until the callbacks is fully executed as mocha defined. However, without the done callbacks, the tests seem to function as expected |
|
Hi @arthurschreiber, Did a bit investigation on the random test failures under sender-test. Seems it is caused by an issue within dgram.js. To fully avoid this random failure, according to this ticket: nodejs/node#7061, I think we could either try to modify the sender.ts based on the workarounds mentioned in the ticket or checking whether the node side going to fix this. People have created a PR: nodejs/node#7066 for this fix, but the merging process for this PR got complicated on the node side. |
|
🎉 This issue has been resolved in version 8.3.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
refactor some pervious mocha test which is causing some random failures during either travis and appveryor.