Add a unit test for the overriding behavior of sendLoadedSourceEvent()#271
Add a unit test for the overriding behavior of sendLoadedSourceEvent()#271roblourens merged 1 commit intomicrosoft:masterfrom
Conversation
|
@digeff PTAL |
| const loadedSourceEventReason: LoadedSourceEventReason = 'changed'; | ||
| sendEventHandler = (event) => { | ||
| assert.equal('loadedSource', event.event); | ||
| assert.notEqual(null, event.body); |
There was a problem hiding this comment.
Would the test catch more potential issues if we tested the contents of all the members of the event argument?
There was a problem hiding this comment.
Should we keep the tests atomic and follow the one-thing-at-a-time principle? Or otherwise, when the product code changes, we might have to update all the tests, which is tedious and does not gain us anything.
There was a problem hiding this comment.
Ideally the test should only fail when the contract/interface changes. If we change the implementation while maintaining the original contract/interface then the test shouldn't fail nor need updating.
There was a problem hiding this comment.
Resolved offline. @digeff and I both agree on this change now.
| assert.equal('loadedSource', event.event); | ||
| assert.notEqual(null, event.body); | ||
| assert.equal(loadedSourceEventReason, event.body.reason); | ||
| done(); |
There was a problem hiding this comment.
How will done be called if the test fails?
There was a problem hiding this comment.
It will timeout and show as a timeout failure.
There was a problem hiding this comment.
It'd be great if we could show the actual failure rather than a timeout.
There was a problem hiding this comment.
Updated to return the promise directly instead of using done method.
3 participants
Added 1 more test.