test:Refactor DNS upstream resolver to use interface-based client and improve test coverage#1554
test:Refactor DNS upstream resolver to use interface-based client and improve test coverage#1554adity1raut wants to merge 1 commit intokmesh-net:mainfrom
Conversation
…t tests for resolver functionality Signed-off-by: adity1raut <araut7798@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @adity1raut, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testability and robustness of the DNS upstream resolver. By introducing an interface for the DNS client, it removes direct dependencies on external network interactions, allowing for more isolated and reliable unit testing. This refactoring improves the overall code quality and ensures the DNS resolution logic behaves as expected under various conditions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Interface defined, Tests now run, clean and swift, Code gains strength anew. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the upstreamResolver to use an interface for the DNS client, which is a great improvement for testability. The introduction of fakeDNSClient and the new unit tests significantly improve the test coverage for the upstream resolution logic.
My main feedback is regarding the error handling strategy in upstreamResolver.Resolve. When all upstream servers fail (e.g., due to network errors), the function currently swallows these errors and returns a synthetic ServerFailure response. This masks the true cause of the failure from callers. I've added a specific comment in upstream_test.go with a suggestion to propagate the error in such scenarios, which would make the resolver more robust and easier to debug. The new test TestResolve_AllUpstreamsFail should be adjusted to reflect this change.
| assert.NoError(t, err) | ||
| assert.Equal(t, dns.RcodeServerFailure, out.Rcode) |
There was a problem hiding this comment.
This test asserts that Resolve returns NoError when all upstreams fail. However, the Resolve function in upstream.go should not swallow errors. When all upstreams are unreachable (e.g., due to network issues), it should propagate an error instead of returning a synthetic RcodeServerFailure response with a nil error. Hiding the error makes it difficult for callers to understand the root cause of the failure.
Please modify upstreamResolver.Resolve to return an error when all upstream queries fail. Consequently, this test should be updated to assert that an error is returned and the response is nil.
| assert.NoError(t, err) | |
| assert.Equal(t, dns.RcodeServerFailure, out.Rcode) | |
| assert.Error(t, err) | |
| assert.Nil(t, out) |
There was a problem hiding this comment.
Pull request overview
This PR refactors the DNS upstream resolver to improve testability by introducing an interface-based design pattern. It replaces the embedded *dns.Client with a dnsClient interface, enabling clean dependency injection and comprehensive unit testing without method overriding.
- Introduced a
dnsClientinterface to abstract DNS exchange behavior - Refactored
upstreamResolverto use the interface instead of embedding*dns.Client - Added comprehensive unit tests with a fake DNS client for deterministic testing scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/dns/upstream.go | Introduces dnsClient interface and updates upstreamResolver to use it as a field instead of embedding *dns.Client |
| pkg/dns/upstream_test.go | Adds new test file with fakeDNSClient implementation and three test cases covering success, fallback, and failure scenarios, plus a test for WithUpstreams method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| responses: map[string]*dns.Msg{ | ||
| "1.1.1.1:53": resp, | ||
| }, | ||
| errors: map[string]error{}, |
There was a problem hiding this comment.
The empty map initialization on this line is unnecessary. In Go, when you check a map with the comma-ok idiom (as done in the Exchange method), a nil map behaves the same as an empty map for lookups. You can safely remove this line to simplify the test setup.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
/cc @hzxuzhonghu , @LiZhenCheng9527 PTAL |
2 participants
What type of PR is this?
/kind enhancement
/kind security
/kind feature
What this PR does / why we need it:
This PR refactors the DNS upstream resolver to improve testability and code quality by introducing an interface-based DNS client. It removes direct dependency on *dns.Client, enabling clean mocking in unit tests without method overriding.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: