Begin integrating ixfr package and stupidns package #107
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=error msg="Running error: context loading failed: no go files to analyze: running WalkthroughThe pull request introduces a comprehensive testing framework for a DNS-related project, focusing on creating a lightweight DNS server called StupiDNS and implementing Incremental Zone Transfer (IXFR) functionality. The changes span multiple packages, including Changes
Sequence DiagramsequenceDiagram
participant Client
participant StupiDNS
participant TestFramework
TestFramework->>StupiDNS: Create Server
TestFramework->>StupiDNS: Queue Responses
TestFramework->>StupiDNS: Start Server
Client->>StupiDNS: Send DNS Query
StupiDNS-->>Client: Return Queued Response
TestFramework->>TestFramework: Validate Response
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (17)
tdns/ixfr/ixfr.go (2)
1-2: Ensure consistent naming conventions.
You're usingixfrfor both the package name and the struct name. Although not strictly an error, consider updating one or the other to make references clearer within the code and documentation.
124-138: RevisitGetCompressednaming.
The method merges multipleDiffSequenceobjects into one aggregated list, returning a new sequence with combinedAddedRecordsandDeletedRecords. The name is valid but consider something more descriptive likeMergeDiffSequencesorFlattenDiffs.stupidns/result_fetchers.go (1)
4-43: Consider refactoring repetitive slice initialization logic.The test utility functions follow a similar pattern of slice initialization. Consider introducing a helper function to reduce code duplication.
+func makeStringSlice(values ...string) []string { + ret := make([]string, len(values)) + copy(ret, values) + return ret +} func getFoo(obj ...interface{}) []string { - ret := make([]string, 1) - ret[0] = "foo" - return ret + return makeStringSlice("foo") } func getBarFoo(obj ...interface{}) []string { - ret := make([]string, 2) - ret[0] = "bar" - ret[1] = "foo" - return ret + return makeStringSlice("bar", "foo") }tdns/ixfr/diffsequence.go (2)
78-131: Improve error handling and simplify slice operations.A few suggestions for improvement:
- The slice operation
slice[1:len(slice)]can be simplified toslice[1:]- Error messages could be more descriptive by including the actual error and record details
- Consider pre-allocating the results slice with an estimated capacity
- diff[key] = slice[1:len(slice)] + diff[key] = slice[1:]- panic("Error calculating diff between RR slices") + panic(fmt.Sprintf("Error calculating diff between RR slices: %v for record %s", err, s))- rrs := make([]dns.RR, 0) + rrs := make([]dns.RR, 0, len(diff))🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: S1010: should omit second index in slice, s[a:len(s)] is identical to s[a:]
(gosimple)
49-71: Enhance error handling in Add methods.The error handling in both
AddAddedandAddDeletedcould be more informative by including the failing record string in the panic message.- panic("Error adding RR to 'added' slice") + panic(fmt.Sprintf("Error adding RR to 'added' slice: %v for record %s", err, rrStr))- panic("Error adding RR to 'deleted' slice") + panic(fmt.Sprintf("Error adding RR to 'deleted' slice: %v for record %s", err, rrStr))tdns/ixfr/diffsequence_test.go (2)
7-21: Consider using table-driven tests for better test organization.The
TestDiffSequenceEqualstest could be refactored into a table-driven test to make it easier to add more test cases.func TestDiffSequenceEquals(t *testing.T) { - seq1 := CreateDiffSequence(2, 3) - seq1.AddAdded("nezu.jain.ad.jp A 133.69.136.5") - seq1.AddDeleted("jain-bb.jain.ad.jp A 133.69.136.4") - seq1.AddDeleted("jain-bb.jain.ad.jp A 192.41.197.2") - - seq2 := CreateDiffSequence(2, 3) - seq2.AddDeleted("jain-bb.jain.ad.jp A 192.41.197.2") - seq2.AddDeleted("jain-bb.jain.ad.jp A 133.69.136.4") - seq2.AddAdded("nezu.jain.ad.jp A 133.69.136.5") - - if !seq1.Equals(seq2) { - t.Errorf("Sequences not equal!") - } + tests := []struct { + name string + seq1 DiffSequence + seq2 DiffSequence + wantEqual bool + }{ + { + name: "same records different order", + seq1: func() DiffSequence { + seq := CreateDiffSequence(2, 3) + seq.AddAdded("nezu.jain.ad.jp A 133.69.136.5") + seq.AddDeleted("jain-bb.jain.ad.jp A 133.69.136.4") + seq.AddDeleted("jain-bb.jain.ad.jp A 192.41.197.2") + return seq + }(), + seq2: func() DiffSequence { + seq := CreateDiffSequence(2, 3) + seq.AddDeleted("jain-bb.jain.ad.jp A 192.41.197.2") + seq.AddDeleted("jain-bb.jain.ad.jp A 133.69.136.4") + seq.AddAdded("nezu.jain.ad.jp A 133.69.136.5") + return seq + }(), + wantEqual: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.seq1.Equals(tt.seq2); got != tt.wantEqual { + t.Errorf("DiffSequence.Equals() = %v, want %v", got, tt.wantEqual) + } + }) + }
36-38: Improve test error messages.The error message could be more descriptive by including the actual and expected values.
- t.Errorf("Got: %+v\n Want: %+v\n", got, want) + t.Errorf("GetAdded() got = %+v, want %+v", got, want)stupidns/common.go (1)
68-77: Consider using error wrapping instead of panic.The
SetupandTeardownfunctions use panic for error handling. Consider returning errors instead to allow better error handling by callers.-func Setup() { +func Setup() error { var err error TEST_RUNDIR, err = os.MkdirTemp("", "tdns_integration_test_*") TEST_RUNDIR += "/" if err != nil { - panic("Error setting up integration tests") + return fmt.Errorf("error setting up integration tests: %w", err) } + return nil } -func Teardown() { +func Teardown() error { err := os.RemoveAll(TEST_RUNDIR) if err != nil { - panic("Error tearing down integration tests") + return fmt.Errorf("error tearing down integration tests: %w", err) } + return nil }Also applies to: 79-86
stupidns/result_checkers.go (1)
28-58: Consider pre-allocating the map with the correct size.The
unorderedSliceComparefunction could be optimized by pre-allocating the map with the correct size.- diff := make(map[string]int, len(got)) + // Pre-allocate with size of non-empty strings + size := 0 + for _, g := range got { + if g != "" { + size++ + } + } + diff := make(map[string]int, size)tdns/ixfr/ixfr_test.go (2)
9-47: Consider using table-driven tests for better maintainability.The test data could be structured as a table of test cases, making it easier to add more test scenarios.
Here's an example refactor:
+type ixfrTestCase struct { + name string + response []string + want Ixfr +} + func TestIxfr(t *testing.T) { - /* Should be equivalent to the example in RFC 1995 */ - response := new(dns.Msg) - response.Answer = makeRRSlice( - "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800", + tests := []ixfrTestCase{ + { + name: "RFC 1995 example", + response: []string{ + "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800", + // ... other records + }, + want: func() Ixfr { + wanted := Ixfr{ + InitialSOASerial: 1, + FinalSOASerial: 3, + DiffSequences: []DiffSequence{}, + } + // ... setup wanted state + return wanted + }(), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + response := new(dns.Msg) + response.Answer = makeRRSlice(tc.response...) + got := IxfrFromResponse(response) + if !got.Equals(tc.want) { + t.Errorf("Got:\n %+v\n Want:\n %+v", got, tc.want) + } + }) + }
49-78: Reduce code duplication in test data setup.TestGetAdded and TestGetDeleted use the same test data setup. Consider extracting the common setup into a helper function.
Example refactor:
+func setupTestResponse() *dns.Msg { + response := new(dns.Msg) + response.Answer = makeRRSlice( + "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800", + // ... other records + ) + return response +} + func TestGetAdded(t *testing.T) { - /* Should be equivalent to the example in RFC 1995 */ - response := new(dns.Msg) - response.Answer = makeRRSlice( + response := setupTestResponse()Also applies to: 80-106
tests/Makefile (1)
1-3: LGTM! Consider adding test parallelization.The integration test target is well-configured with appropriate flags. Consider adding the
-parallelflag to control test parallelization, especially since DNS tests might have port conflicts when run in parallel..PHONY: integration integration: - go test -count=1 -tags integration -run TestIntegration* -v ./... + go test -count=1 -parallel 1 -tags integration -run TestIntegration* -v ./...tests/testdata/sanity_basic_ordered.yaml (1)
1-11: Add test documentation and ensure consistent port allocation.The test configuration needs documentation and port allocation strategy:
- Add descriptive comments about the ordered comparison test case
- Consider using a port allocation strategy across test files
+# Test case: Ordered Comparison +# Purpose: Validates that getFooBar fetcher returns results in correct order +# Expected: Returns ["foo", "bar"] in exact order mqueue: - [] # Server not used in this test expected: - "foo" - "bar" fetcher: "getFooBar" checker: "orderedSliceCompare" runs: 1Consider implementing a central port allocation mechanism in the test framework to prevent conflicts and make port management more maintainable.
tests/testdata/sanity_basic_unordered.yaml (1)
1-11: Document test case and consider test organization.The test configuration needs:
- Documentation explaining the unordered comparison test case
- Consider organizing related test cases in a structured way
+# Test case: Unordered Comparison +# Purpose: Validates that order-independent comparison works correctly +# Expected: Returns ["foo", "bar"] in any order using getBarFoo fetcher mqueue: - [] # Server not used in this test expected: - "foo" - "bar" fetcher: "getBarFoo" checker: "unorderedSliceCompare" runs: 1Consider organizing related test cases (basic, ordered, unordered) in a structured way:
- Group related test files in subdirectories
- Use a common configuration for shared settings
- Implement test case dependencies if needed
tests/testdata/ixfr_integration_two_records.yaml (2)
4-5: Consider using example.test instead of example.comThe test currently uses example.com which is a reserved domain. Consider using example.test which is specifically reserved for testing purposes according to RFC 6761.
-- - "example.com. 190 IN A 23.192.228.84" - - "example.com. 300 IN A 96.7.128.175" ++ - "example.test. 190 IN A 23.192.228.84" + - "example.test. 300 IN A 96.7.128.175"
13-14: Document port significanceThe specific port number (53530) might have significance for IXFR integration tests. Consider adding a comment explaining why this port was chosen.
tests/testdata/simple_request_two_records.yaml (1)
1-3: Enhance test documentationThe comment should clarify how this "simple request" test differs from the IXFR integration test, as they currently appear to test the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
stupidns/go.sumis excluded by!**/*.sumtdns/ixfr/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
stupidns/common.go(1 hunks)stupidns/go.mod(1 hunks)stupidns/result_checkers.go(1 hunks)stupidns/result_fetchers.go(1 hunks)stupidns/stupidns.go(1 hunks)tdns/ixfr/Makefile(1 hunks)tdns/ixfr/diffsequence.go(1 hunks)tdns/ixfr/diffsequence_test.go(1 hunks)tdns/ixfr/go.mod(1 hunks)tdns/ixfr/ixfr.go(1 hunks)tdns/ixfr/ixfr_test.go(1 hunks)tdns/ixfr/util.go(1 hunks)tdns/ixfr/util_test.go(1 hunks)tests/Makefile(1 hunks)tests/README.md(1 hunks)tests/go.mod(1 hunks)tests/ixfr_integration_test.go(1 hunks)tests/main_test.go(1 hunks)tests/sanity_test.go(1 hunks)tests/simple_test.go(1 hunks)tests/testdata/ixfr_integration_two_records.yaml(1 hunks)tests/testdata/sanity_basic.yaml(1 hunks)tests/testdata/sanity_basic_ordered.yaml(1 hunks)tests/testdata/sanity_basic_unordered.yaml(1 hunks)tests/testdata/simple_request.yaml(1 hunks)tests/testdata/simple_request_two_records.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/README.md
- tests/go.mod
- stupidns/go.mod
- tdns/ixfr/go.mod
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tdns/ixfr/diffsequence.go
109-109: S1010: should omit second index in slice, s[a:len(s)] is identical to s[a:]
(gosimple)
stupidns/common.go
26-26: structtag: struct field tag yaml:fetcher not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
27-27: structtag: struct field tag yaml:checker not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
28-28: structtag: struct field tag yaml:runs not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
29-29: structtag: struct field tag yaml:config not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
stupidns/stupidns.go
82-82: Error return value of s.srv.Shutdown is not checked
(errcheck)
45-45: copylocks: return copies lock value: github.com/zluudg/tdns/stupidns.StupiDNS contains github.com/miekg/dns.Server contains sync.RWMutex
(govet)
tdns/ixfr/ixfr.go
9-9: var logger is unused
(unused)
🔇 Additional comments (23)
tdns/ixfr/ixfr.go (8)
3-7: Consider selective imports.
The import block is straightforward, but you might want to import only what’s necessary to reduce potential import bloat. If you plan to expand usage offmtor rely on other sub-packages fromgithub.com/miekg/dns, keep as is.
11-17: Struct definition looks good.
TheIxfrstruct captures the key fields for incremental transfers, includingInitialSOASerial,FinalSOASerial, and the diff sequences. This is clear and comprehensible.
19-21: Revisit public API usage ofSetLoggerHandle.
Great approach to allow external injection of a logging function. But currently, it's not used anywhere else in this file. If you plan to log inIxfrFromResponseor other methods, integrate it, or remove the logger entirely.
23-25: Straightforward method.
AddDiffSequenceis concise and clean.
27-47: EnsureEqualshandles all fields consistently.
The equality check is thorough forInitialSOASerial,FinalSOASerial, and eachDiffSequence. Make sure to also confirm if you want to compareIsAxfrandAxfrRRsfor equality in the future.
140-143: Good aggregator for added records.
GetAddedsimply defers toGetCompressed()and returns the “added” portion. Straightforward and consistent.
145-148: Consistent aggregator for deleted records.
Similar toGetAdded,GetDeletedis a pass-through to aggregated data. This is consistent.
49-122: 🛠️ Refactor suggestionImprove safety and error handling in
IxfrFromResponse.
- Potential index out-of-range: You directly access
rsp.Answer[1]at line 55 andrsp.Answer[0]at line 63. Ifrsp.Answeris shorter than expected, it will panic.- Use of
panic: Consider returning an error rather than callingpanicin production/library code. Panics can crash the entire program, whereas returning an error allows consumers to handle the case gracefully.- Toggle logic: The
_isAdding_toggle between added and deleted records is well-structured. Ensure test coverage for boundary conditions when switching states.- Line 88: The block comment references an exception for “the first SOA in the diff sequence list,” which is a subtle edge case. Ensure you have a test that covers the scenario where the first record and the last record are SOAs.
tests/main_test.go (3)
1-3: Build constraint verified.
Specifying//go:build integrationensures this file is only included in the integration build. This is a helpful organizational approach for test environments.
4-9: Package and imports look fine.
Thetestspackage dedicated to integration tests is easy to track. Dependence onosandtestingis minimal and appropriate.
11-14: UseTestMainfor test setup/teardown.
This function cleanly delegates test execution tom.Run(). If you need heavier initialization or cleanup steps in the future, this is the right place for it.tdns/ixfr/util.go (3)
1-2: Package boundary noted.
Keeping all IXFR helper functions within the sameixfrpackage is sensible.
3-5: Minimal imports are appropriate.
github.com/miekg/dnsis the key dependency here, so no concerns.
21-49: Pragmatic record comparison inrrEquals.
- Using a map to count occurrences is an efficient and clean approach to test equality with duplicates.
- Consider consolidating
nilchecks or proactively filteringnilitems to keep the logic simpler.tests/sanity_test.go (1)
69-71:⚠️ Potential issueEmpty test run loop requires implementation.
The test run loop is empty and needs to be implemented.
tdns/ixfr/diffsequence.go (2)
9-23: LGTM! Clean struct design with proper initialization.The
DiffSequencestruct and its constructor are well-designed, with clear field names and proper initialization of empty slices.
73-77: Consider documenting the current behavior for differing record counts.The TODO comment raises an important question about handling different numbers of added/deleted records. Until this is implemented, it's crucial to document the current behavior to prevent confusion.
stupidns/result_checkers.go (1)
11-23: Optimize slice comparison by early length check.The
orderedSliceComparefunction could be more efficient by checking lengths before iterating.The implementation is correct and well-documented.
tests/ixfr_integration_test.go (1)
41-44: Apply the same fixes as in simple_test.go.This file has the same issues as
simple_test.go:
- YAML unmarshaling error not properly handled
- Connection retry mechanism needs improvement
- Response validation using panic instead of t.Fatal
Please apply the same fixes as suggested in the
simple_test.goreview.Also applies to: 77-91, 101-106
tdns/ixfr/Makefile (1)
1-3: LGTM!The Makefile correctly defines the test target and marks it as .PHONY.
tests/testdata/sanity_basic.yaml (1)
1-10: Add test case documentation and port validation.While the test configuration is valid, consider:
- Adding a comment block explaining the test's purpose and expected behavior
- Implementing port availability checks before test execution
+# Test case: Basic Functionality +# Purpose: Validates the basic fetcher and checker functionality +# Expected: Returns "foo" using getFoo fetcher mqueue: - [] # Server not used in this test expected: - "foo" fetcher: "getFoo" checker: "orderedSliceCompare" runs: 1Let's verify port availability across all test files:
✅ Verification successful
Add test case documentation to improve maintainability
The port allocation is well-managed with unique sequential ports (53530-53535) across test files. Consider adding the suggested documentation to improve test maintainability:
+# Test case: Basic Functionality +# Purpose: Validates the basic fetcher and checker functionality +# Expected: Returns "foo" using getFoo fetcher mqueue: - [] # Server not used in this test expected: - "foo"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for port conflicts in test configurations echo "Checking for port conflicts in test configurations..." rg -l "port: \"[0-9]+\"" tests/testdata/ for file in $(rg -l "port: \"[0-9]+\"" tests/testdata/); do echo "Ports in $file:" rg "port: \"([0-9]+)\"" "$file" -o -r '$1' doneLength of output: 1959
tests/testdata/simple_request.yaml (1)
1-12: Consider DNS-specific validations and edge cases.While the test case is well-documented, consider:
- Adding validation for DNS record format
- Including edge cases (e.g., CNAME resolution, multiple A records)
- Testing TTL boundary conditions
Add additional test cases:
# Test case: Multiple A Records mqueue: - - "example.com. 190 IN A 23.192.228.84" - "example.com. 190 IN A 23.192.228.85" expected: - "23.192.228.84" - "23.192.228.85" fetcher: "getFromBucket" checker: "orderedSliceCompare" runs: 1 # Test case: CNAME Resolution mqueue: - - "example.com. 190 IN CNAME cdn.example.com." - "cdn.example.com. 190 IN A 23.192.228.84" expected: - "23.192.228.84" fetcher: "getFromBucket" checker: "orderedSliceCompare" runs: 1Let's verify the DNS record format:
tests/testdata/ixfr_integration_two_records.yaml (1)
6-8: Consider validating TTL valuesThe test only validates IP addresses but ignores TTL values. Consider adding TTL validation if these values are significant for IXFR functionality.
Would you like me to propose an extended test case that includes TTL validation?
| "github.com/miekg/dns" | ||
| ) | ||
|
|
||
| var logger = fmt.Printf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or use the logger variable.
The static analysis tool flags logger as unused. If you don't plan to use it, remove it to reduce clutter.
-var logger = fmt.Printf
+// var logger = fmt.Printf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var logger = fmt.Printf | |
| // var logger = fmt.Printf |
🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: var logger is unused
(unused)
| func makeRRSlice(rrs ...string) []dns.RR { | ||
| rrSlice := make([]dns.RR, len(rrs)) | ||
|
|
||
| for i, r := range rrs { | ||
| rr, err := dns.NewRR(r) | ||
| if err != nil { | ||
| panic("Oh no, could not create list!") | ||
| } | ||
| rrSlice[i] = rr | ||
| } | ||
|
|
||
| return rrSlice | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panic in makeRRSlice.
While convenient in a test or prototype scenario, in production code consider returning an error instead of panicking. This will give calling functions more control over error handling.
-func makeRRSlice(rrs ...string) []dns.RR {
+func makeRRSlice(rrs ...string) ([]dns.RR, error) {
rrSlice := make([]dns.RR, len(rrs))
...
if err != nil {
- panic("Oh no, could not create list!")
+ return nil, fmt.Errorf("could not create RR: %v", err)
}
...
- return rrSlice
+ return rrSlice, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func makeRRSlice(rrs ...string) []dns.RR { | |
| rrSlice := make([]dns.RR, len(rrs)) | |
| for i, r := range rrs { | |
| rr, err := dns.NewRR(r) | |
| if err != nil { | |
| panic("Oh no, could not create list!") | |
| } | |
| rrSlice[i] = rr | |
| } | |
| return rrSlice | |
| } | |
| func makeRRSlice(rrs ...string) ([]dns.RR, error) { | |
| rrSlice := make([]dns.RR, len(rrs)) | |
| for i, r := range rrs { | |
| rr, err := dns.NewRR(r) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create RR: %v", err) | |
| } | |
| rrSlice[i] = rr | |
| } | |
| return rrSlice, nil | |
| } |
There was a problem hiding this comment.
I agree. Let's try to avoid panic() and log.Fatalf() in the library itself.
There was a problem hiding this comment.
@johanix Thank you for confirming. I'll make sure to flag any usage of panic() or log.Fatalf() in library code in future reviews.
(｀^´)=͟͟͞͞🥕
✏️ Learnings added
Learnt from: johanix
PR: johanix/tdns#107
File: tdns/ixfr/util.go:7-19
Timestamp: 2025-02-03T16:12:44.452Z
Learning: In the tdns library code, avoid using panic() or log.Fatalf(). Instead, functions should return errors to allow proper error handling by the calling code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func getFromBucket(obj ...interface{}) []string { | ||
| if len(obj) != 1 { | ||
| panic("\"getFromBucket\" requires one argument!") | ||
| } | ||
|
|
||
| var b Bucket = obj[0].(Bucket) | ||
| return b.Get() | ||
| } |
There was a problem hiding this comment.
Add type assertion error handling in getFromBucket.
The type assertion obj[0].(Bucket) will panic if the assertion fails. Consider adding proper error handling.
func getFromBucket(obj ...interface{}) []string {
if len(obj) != 1 {
panic("\"getFromBucket\" requires one argument!")
}
- var b Bucket = obj[0].(Bucket)
+ b, ok := obj[0].(Bucket)
+ if !ok {
+ panic(fmt.Sprintf("Expected Bucket type, got %T", obj[0]))
+ }
return b.Get()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getFromBucket(obj ...interface{}) []string { | |
| if len(obj) != 1 { | |
| panic("\"getFromBucket\" requires one argument!") | |
| } | |
| var b Bucket = obj[0].(Bucket) | |
| return b.Get() | |
| } | |
| func getFromBucket(obj ...interface{}) []string { | |
| if len(obj) != 1 { | |
| panic("\"getFromBucket\" requires one argument!") | |
| } | |
| b, ok := obj[0].(Bucket) | |
| if !ok { | |
| panic(fmt.Sprintf("Expected Bucket type, got %T", obj[0])) | |
| } | |
| return b.Get() | |
| } |
There was a problem hiding this comment.
@johanix Thanks for confirming! I'll make sure to remember this pattern for future reviews.
✏️ Learnings added
Learnt from: johanix
PR: johanix/tdns#107
File: stupidns/result_fetchers.go:45-52
Timestamp: 2025-02-03T16:13:42.215Z
Learning: In Go code, type assertions should include error handling with descriptive panic messages, using the pattern: value, ok := x.(Type); if !ok { panic(fmt.Sprintf("Expected Type, got %T", x)) }
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| func TestRrEqualsWhitespaceDiff(t *testing.T) { | ||
| want := make([]dns.RR, 0) | ||
| rr, _ := dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | ||
| want = append(want, rr) | ||
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | ||
| want = append(want, rr) | ||
|
|
||
| otherWant := make([]dns.RR, 0) | ||
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | ||
| otherWant = append(otherWant, rr) | ||
| rr, _ = dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | ||
| otherWant = append(otherWant, rr) | ||
|
|
||
| if !rrEquals(want, otherWant) { | ||
| t.Errorf("dns.RR Equality test failed!") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply similar improvements to TestRrEqualsWhitespaceDiff.
The same issues with error handling and test structure exist in this test function.
Consider applying the same table-driven test pattern and error handling improvements as suggested for TestRrEquals.
| func TestRrEquals(t *testing.T) { | ||
| want := make([]dns.RR, 0) | ||
| rr, _ := dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | ||
| want = append(want, rr) | ||
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | ||
| want = append(want, rr) | ||
|
|
||
| otherWant := make([]dns.RR, 0) | ||
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | ||
| otherWant = append(otherWant, rr) | ||
| rr, _ = dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | ||
| otherWant = append(otherWant, rr) | ||
|
|
||
| if !rrEquals(want, otherWant) { | ||
| t.Errorf("dns.RR Equality test failed!") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and test structure in TestRrEquals.
The test function ignores potential errors from dns.NewRR and uses a non-descriptive error message.
func TestRrEquals(t *testing.T) {
- want := make([]dns.RR, 0)
- rr, _ := dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800")
- want = append(want, rr)
- rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3")
- want = append(want, rr)
+ tests := []struct {
+ name string
+ records1 []string
+ records2 []string
+ want bool
+ }{
+ {
+ name: "same records different order",
+ records1: []string{
+ "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800",
+ "jain-bb.jain.ad.jp A 133.69.136.3",
+ },
+ records2: []string{
+ "jain-bb.jain.ad.jp A 133.69.136.3",
+ "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800",
+ },
+ want: true,
+ },
+ }
- otherWant := make([]dns.RR, 0)
- rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3")
- otherWant = append(otherWant, rr)
- rr, _ = dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800")
- otherWant = append(otherWant, rr)
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var want, otherWant []dns.RR
+ for _, r := range tt.records1 {
+ rr, err := dns.NewRR(r)
+ if err != nil {
+ t.Fatalf("Failed to create RR: %v", err)
+ }
+ want = append(want, rr)
+ }
+ for _, r := range tt.records2 {
+ rr, err := dns.NewRR(r)
+ if err != nil {
+ t.Fatalf("Failed to create RR: %v", err)
+ }
+ otherWant = append(otherWant, rr)
+ }
- if !rrEquals(want, otherWant) {
- t.Errorf("dns.RR Equality test failed!")
+ if got := rrEquals(want, otherWant); got != tt.want {
+ t.Errorf("rrEquals() = %v, want %v", got, tt.want)
+ }
+ })
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestRrEquals(t *testing.T) { | |
| want := make([]dns.RR, 0) | |
| rr, _ := dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | |
| want = append(want, rr) | |
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | |
| want = append(want, rr) | |
| otherWant := make([]dns.RR, 0) | |
| rr, _ = dns.NewRR("jain-bb.jain.ad.jp A 133.69.136.3") | |
| otherWant = append(otherWant, rr) | |
| rr, _ = dns.NewRR("jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800") | |
| otherWant = append(otherWant, rr) | |
| if !rrEquals(want, otherWant) { | |
| t.Errorf("dns.RR Equality test failed!") | |
| } | |
| } | |
| func TestRrEquals(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| records1 []string | |
| records2 []string | |
| want bool | |
| }{ | |
| { | |
| name: "same records different order", | |
| records1: []string{ | |
| "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800", | |
| "jain-bb.jain.ad.jp A 133.69.136.3", | |
| }, | |
| records2: []string{ | |
| "jain-bb.jain.ad.jp A 133.69.136.3", | |
| "jain.ad.jp SOA NS.JAIN.AD.JP. mohta.jain.ad.jp. 3 600 600 3600000 604800", | |
| }, | |
| want: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| var want, otherWant []dns.RR | |
| for _, r := range tt.records1 { | |
| rr, err := dns.NewRR(r) | |
| if err != nil { | |
| t.Fatalf("Failed to create RR: %v", err) | |
| } | |
| want = append(want, rr) | |
| } | |
| for _, r := range tt.records2 { | |
| rr, err := dns.NewRR(r) | |
| if err != nil { | |
| t.Fatalf("Failed to create RR: %v", err) | |
| } | |
| otherWant = append(otherWant, rr) | |
| } | |
| if got := rrEquals(want, otherWant); got != tt.want { | |
| t.Errorf("rrEquals() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
| type Case struct { | ||
| Mqueue [][]string `yaml:"mqueue"` | ||
| Expected []string `yaml:"expected"` | ||
| Fetcher string `yaml:fetcher` | ||
| Checker string `yaml:checker` | ||
| Runs uint `yaml:runs` | ||
| Config map[string]string `yaml:config` | ||
| } |
There was a problem hiding this comment.
Fix YAML struct tag syntax.
The YAML struct tags are missing quotes, which makes them incompatible with reflect.StructTag.Get.
type Case struct {
Mqueue [][]string `yaml:"mqueue"`
Expected []string `yaml:"expected"`
- Fetcher string `yaml:fetcher`
- Checker string `yaml:checker`
- Runs uint `yaml:runs`
- Config map[string]string `yaml:config`
+ Fetcher string `yaml:"fetcher"`
+ Checker string `yaml:"checker"`
+ Runs uint `yaml:"runs"`
+ Config map[string]string `yaml:"config"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Case struct { | |
| Mqueue [][]string `yaml:"mqueue"` | |
| Expected []string `yaml:"expected"` | |
| Fetcher string `yaml:fetcher` | |
| Checker string `yaml:checker` | |
| Runs uint `yaml:runs` | |
| Config map[string]string `yaml:config` | |
| } | |
| type Case struct { | |
| Mqueue [][]string `yaml:"mqueue"` | |
| Expected []string `yaml:"expected"` | |
| Fetcher string `yaml:"fetcher"` | |
| Checker string `yaml:"checker"` | |
| Runs uint `yaml:"runs"` | |
| Config map[string]string `yaml:"config"` | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
26-26: structtag: struct field tag yaml:fetcher not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
27-27: structtag: struct field tag yaml:checker not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
28-28: structtag: struct field tag yaml:runs not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
29-29: structtag: struct field tag yaml:config not compatible with reflect.StructTag.Get: bad syntax for struct tag value
(govet)
| yaml.Unmarshal(file, &c) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("Error unmarshaling testdata file: %s", err)) | ||
| } |
There was a problem hiding this comment.
Handle YAML unmarshaling error.
The error from yaml.Unmarshal is not being checked. The current error check uses an undefined err variable.
Apply this diff to fix the error handling:
-yaml.Unmarshal(file, &c)
-if err != nil {
+if err := yaml.Unmarshal(file, &c); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| yaml.Unmarshal(file, &c) | |
| if err != nil { | |
| panic(fmt.Sprintf("Error unmarshaling testdata file: %s", err)) | |
| } | |
| if err := yaml.Unmarshal(file, &c); err != nil { | |
| panic(fmt.Sprintf("Error unmarshaling testdata file: %s", err)) | |
| } |
| if r == nil { | ||
| panic(fmt.Sprintf("*** error: %s\n", err)) | ||
| } | ||
| if r.Rcode != dns.RcodeSuccess { | ||
| panic("Bad response code!") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in response validation.
The current code uses panic for error handling, which is not idiomatic in Go tests. Use t.Fatal instead.
Apply this diff to improve error handling:
-if r == nil {
- panic(fmt.Sprintf("*** error: %s\n", err))
-}
-if r.Rcode != dns.RcodeSuccess {
- panic("Bad response code!")
-}
+if r == nil {
+ t.Fatalf("Failed to get response: %v", err)
+}
+if r.Rcode != dns.RcodeSuccess {
+ t.Fatalf("Bad response code: %v", r.Rcode)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if r == nil { | |
| panic(fmt.Sprintf("*** error: %s\n", err)) | |
| } | |
| if r.Rcode != dns.RcodeSuccess { | |
| panic("Bad response code!") | |
| } | |
| if r == nil { | |
| t.Fatalf("Failed to get response: %v", err) | |
| } | |
| if r.Rcode != dns.RcodeSuccess { | |
| t.Fatalf("Bad response code: %v", r.Rcode) | |
| } |
| for _ = range c.Runs { | ||
| /* Create the client */ | ||
| client := new(dns.Client) | ||
| client.Net = "tcp" | ||
| var conn *dns.Conn | ||
|
|
||
| /* Retry 10 times in case StupiDNS is not up yet */ | ||
| for range 10 { | ||
| conn, err = client.Dial(addr) | ||
|
|
||
| if err == nil { | ||
| break | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for connection retries.
The retry loop doesn't handle the case where all retries fail, and it doesn't use a proper delay between retries.
Apply this diff to improve the retry mechanism:
-for range 10 {
+for i := 0; i < 10; i++ {
conn, err = client.Dial(addr)
if err == nil {
break
}
+ if i == 9 {
+ panic(fmt.Sprintf("Failed to connect after 10 retries: %v", err))
+ }
+ time.Sleep(100 * time.Millisecond)
}Don't forget to add "time" to the imports.
Committable suggestion skipped: line range outside the PR's diff.
| # A basic test emulating a client getting two A records for a response and then | ||
| # extracts the IP addresses from that response. | ||
| mqueue: | ||
| - - "example.com. 190 IN A 23.192.228.84" | ||
| - "example.com. 300 IN A 96.7.128.175" | ||
| expected: | ||
| - "23.192.228.84" | ||
| - "96.7.128.175" | ||
| fetcher: "getFromBucket" | ||
| checker: "orderedSliceCompare" | ||
| runs: 1 | ||
| config: | ||
| nameserver: "127.0.0.1" | ||
| port: "53534" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing test duplication
This test file is nearly identical to ixfr_integration_two_records.yaml with only the port number being different. Consider:
- Creating a shared test data file for common configurations
- Documenting the distinction between "simple_request" and "ixfr_integration" test purposes
- Explaining the significance of different port numbers (53534 vs 53530)
Example shared structure:
# common_test_data.yaml
base_test:
mqueue:
- - "example.test. 190 IN A 23.192.228.84"
- "example.test. 300 IN A 96.7.128.175"
expected:
- "23.192.228.84"
- "96.7.128.175"
fetcher: "getFromBucket"
checker: "orderedSliceCompare"
runs: 1
config:
nameserver: "127.0.0.1"Then individual test files would only need to specify unique attributes and include the base configuration.
|
I agree with the code rabbit. Good rabbit! :-) |
Began integrating ixfr code from another project
Also integrating small test framework used in same project
Not used in tdns yet, but can be imported and used by other projects
Summary by CodeRabbit
Here are the release notes for this pull request:
New Features
Improvements
Testing
Infrastructure