Add unit tests - #412
Add unit tests#412sean-g-rae wants to merge 7 commits into
Conversation
What: Add unit tests to cover some small gaps that existing in test coverage How: Making no changes to non-test code, this PR is strictly for adding unit tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
============================================
+ Coverage 66.97% 69.81% +2.84%
- Complexity 3085 3242 +157
============================================
Files 198 198
Lines 13691 13691
Branches 2140 2140
============================================
+ Hits 9170 9559 +389
+ Misses 3941 3545 -396
- Partials 580 587 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ibauersachs should I submit this PR to the dnsjava developers for evaluation? No rush just didn't want to miss something on my end. Thanks! |
|
Nothing missed on your end, I just haven't been working on dnsjava this summer and pursued outdoor activities. Come fall, I assume this will change again. Without having looked at the content yet, thanks a lot for your contribution! |
There was a problem hiding this comment.
Remove; this test isn't worth adding as it only covers an irrelevant implementation detail.
| Name name = Name.fromString("example.com."); | ||
| Cache cache = new Cache(); | ||
| Record a = new ARecord(name, DClass.IN, 3600, InetAddress.getByName("127.0.0.1")); | ||
| cache.addRecord(a, Credibility.AUTH_ANSWER, null); |
There was a problem hiding this comment.
Don't use deprecated methods
| // Since we don't have a network, it should eventually fail with "network error" or "host not | ||
| // found" | ||
| // but without a resolver it throws NPE. | ||
| l.run(); |
There was a problem hiding this comment.
This test doesn't do what it promises: the lookup is successful. Both GitHub runners as wells as running the tests locally do have network access. The result is an NXDOMAIN because of the query to a non-existent subdomain.
There was a problem hiding this comment.
remove; this isn't worth adding.
There was a problem hiding this comment.
remove:
- please no reflection
- create test that don't just cover one case, but all paths (at least of one method)
| } | ||
|
|
||
| private static class TestZoneTransferIn extends ZoneTransferIn { | ||
| private List<byte[]> responses; |
|
|
||
| private static class TestZoneTransferIn extends ZoneTransferIn { | ||
| private List<byte[]> responses; | ||
| private MockTCPClient mockClient; |
|
|
||
| @Override | ||
| TCPClient createTcpClient(Duration timeout) throws IOException { | ||
| mockClient = new MockTCPClient(timeout, responses); |
There was a problem hiding this comment.
return directly, remove intermediate variable
| } | ||
|
|
||
| @Test | ||
| void testGetters() { |
There was a problem hiding this comment.
remove, identical to testNewAXFR
| private static class MockTCPClient extends TCPClient { | ||
| private List<byte[]> responses; | ||
| private int responseIndex = 0; | ||
| private byte[] sentData; |
|
I really appreciate the detailed feedback, @ibauersachs ! I think I followed up and resolved all of the issues, but please let me know if I missed anything. Thank you! |
2 participants
What: Add unit tests to cover some small gaps that existing in test coverage
How: Making no changes to non-test code, this PR is strictly for adding unit tests.
Note: there is no associated issue.