Add more target tests, add invalid keyID error code #14
Conversation
Pull Request Test Coverage Report for Build 552944478Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| } | ||
| } | ||
|
|
||
| func TestODoHResolutionWithRealResolver(t *testing.T) { |
There was a problem hiding this comment.
This type of dependency shouldn't be in a unit test. Why did we go with 1.1.1.1 here?
There was a problem hiding this comment.
Likelihood of 1.1.1.1 being unavailable seems extremely low, and I wanted to exercise the resolve(): https://github.com/cloudflare/odoh-server-go/blob/master/resolver.go#L47 codepath. I figured that the benefits of having atleast one test for that codepath far outweighed the risk of creating a 1.1.1.1 dependency.
There was a problem hiding this comment.
I get that, but I think we can exercise that path by mocking things (or wrapping resolve() appropriately). Adding a network dependency doesn't seem to be the right answer.
There was a problem hiding this comment.
re: mocking it, personally I think exercising the networking codepath in resolve() is pretty valuable
There was a problem hiding this comment.
But I can remove it if you think it's fine without.
There was a problem hiding this comment.
Testing the implementation of resolve() isn't something this project should do. We should only be testing that the code that uses resolve is correct, which is why I suggested mocking it.
chris-wood
left a comment
chris-wood
left a comment
There was a problem hiding this comment.
The key ID mismatch change looks good! I'm requesting changes because I don't think it's appropriate for unit tests to hit the network. (If we have a 1.1.1.1 dependency, let's mock it.)
No description provided.