-
Notifications
You must be signed in to change notification settings - Fork 2.3k
contract-tests: fix detectNetwork test and its implementation #4744
Conversation
- guard this.interfactAdapter in implementation - await in tests as per documentation - remove 3rd positional parameter, message, to so expected and got comparison is shown on failures
8838bb7
to
eb9f7b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems basically good, but I'm a bit confused about the reasoning for removing the messages... how would you get error messages if the tests pass on an error? Since the failure case is when there's no error, there won't be any pre-existing error message to pass on, right?
Also one really minor style nitpick. :P
@@ -119,6 +119,10 @@ module.exports = Contract => ({ | |||
}, | |||
|
|||
async detectNetwork() { | |||
// guard interfaceAdapter! | |||
if (!Boolean(this.interfaceAdapter)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a longer way of saying if (!this.interfaceAdapter)
that, IMO, doesn't really add any clarity to it. I would suggest either the shorter way or the more explicit if (this.interfaceAdapter == null)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, in this case I thought the !Boolean(...)
is clearer than the two alternatives. I'll change it to == null here.
Correct, no pre-existing error message to pass on, however there's still an indication that an expected failure didn't happen, like below where I simulated a failure in a test. I think this is acceptable, but will defer to you whether it's acceptable.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, so the message is redundant because it wasn't saying anything not in the default message. Cool. Approved.
This PR is necessary to fix an error hidden behind a bad test. assert.rejects is a promise that should be awaited. Upon fixing this test, it exposed an error in
detectNetwork()
that didn't guard use ofthis.interfaceAdapter
.assert.reject
tests as per documentation. Note: I updated all of them in the current file. Will make another PR to address this issue in other packages.