feat(tests): ✨ Add unit tests for DnsMessage and DnsWireMessageParser
#480
Conversation
…ser` * Introduced `DnsMessageRecursionDesiredTests` to validate the handling of the `RecursionDesired` option in DNS serialization. * Added `DnsWireMessageParserTests` to ensure correct parsing of DNS message headers and EDNS records. * Enhanced `DnsMessageOptions` to include `RecursionDesired` parameter. * Updated `DnsMessage` to utilize the new `RecursionDesired` option in serialization logic. * Created `DnsWireMessageParser` for lightweight parsing of DNS wire-format messages.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review Complete ✅I've completed a comprehensive review of this PR that adds unit tests for Todo List:
Code Quality & Best Practices ✅Positive aspects:
Areas for improvement:
Potential Issues & Bugs
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
- Coverage 65.51% 65.34% -0.17%
==========================================
Files 99 101 +2
Lines 5196 5318 +122
Branches 1076 1107 +31
==========================================
+ Hits 3404 3475 +71
- Misses 1392 1426 +34
- Partials 400 417 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR enhances DNS message handling by introducing support for the RecursionDesired (RD) flag in DNS queries and adds a new lightweight DNS wire-format parser with corresponding unit tests. The changes enable more flexible DNS query construction and provide utilities for parsing DNS message headers and EDNS information.
- Added
RecursionDesiredparameter toDnsMessageOptions(defaults totruefor backward compatibility) - Created
DnsWireMessageParserfor efficient parsing of DNS wire-format messages - Introduced
AsyncIntervalGatethrottling utility for rate limiting
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
DnsClientX/Throttling/AsyncIntervalGate.cs |
New async gate utility for rate limiting DNS queries with configurable intervals |
DnsClientX/ProtocolDnsWire/DnsWireMessageParser.cs |
New lightweight parser for DNS wire-format messages, extracting header flags and EDNS information |
DnsClientX/ProtocolDnsWire/DnsMessage.cs |
Updated serialization methods to respect the RecursionDesired option when building DNS queries |
DnsClientX/DnsMessageOptions.cs |
Extended with RecursionDesired parameter (defaults to true) |
DnsClientX.Tests/DnsWireMessageParserTests.cs |
Unit tests validating header parsing and EDNS record detection |
DnsClientX.Tests/DnsMessageRecursionDesiredTests.cs |
Unit tests verifying correct RD bit handling in DNS wire serialization |
| public static bool TryParseEdns(byte[]? data, out DnsWireEdnsInfo edns) { | ||
| edns = default; | ||
| if (data == null || data.Length < DnsHeaderLength) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. This if statement should include braces around its body.
| for (int i = 0; i < qd; i++) { | ||
| if (!TrySkipName(data, ref offset)) return false; | ||
| if (offset + 4 > data.Length) return false; | ||
| offset += 4; // QTYPE + QCLASS | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| for (int i = 0; i < rrCount; i++) { | ||
| if (!TrySkipName(data, ref offset)) return false; | ||
| if (!TryReadUInt16(data, ref offset, out var type)) return false; | ||
| if (!TryReadUInt16(data, ref offset, out var rrClass)) return false; | ||
| if (!TryReadUInt32(data, ref offset, out _)) return false; // TTL | ||
| if (!TryReadUInt16(data, ref offset, out var rdlen)) return false; | ||
| if (offset + rdlen > data.Length) return false; |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| if (len > 63) { | ||
| return false; | ||
| } | ||
|
|
||
| if (offset + len > buffer.Length) { | ||
| return false; | ||
| } | ||
|
|
||
| offset += len; | ||
| if (++segments > MaxNameSegmentsToSkip) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| private static bool TryReadUInt16(byte[] buffer, ref int offset, out ushort value) { | ||
| value = 0; | ||
| if (buffer == null || offset < 0 || offset + 2 > buffer.Length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. This if statement should include braces around its body.
| public static bool TryParseHeader(byte[]? data, out DnsWireHeaderInfo header) { | ||
| header = default; | ||
| if (data == null || data.Length < DnsHeaderLength) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. This if statement should include braces around its body.
| if (!TryReadUInt16(data, ref offset, out var qd)) return false; | ||
| if (!TryReadUInt16(data, ref offset, out var an)) return false; | ||
| if (!TryReadUInt16(data, ref offset, out var ns)) return false; | ||
| if (!TryReadUInt16(data, ref offset, out var ar)) return false; |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| private static bool TrySkipName(byte[] buffer, ref int offset) { | ||
| int segments = 0; | ||
| while (true) { | ||
| if (buffer == null || offset < 0 || offset >= buffer.Length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| var len = buffer[offset++]; | ||
| if (len == 0) { | ||
| return true; | ||
| } | ||
|
|
||
| // Compression pointer (RFC 1035 4.1.4): 2 bytes total. | ||
| if ((len & 0xC0) == 0xC0) { | ||
| if (offset >= buffer.Length) { | ||
| return false; | ||
| } | ||
| offset++; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
According to the C# coding guidelines, braces should be preferred even for single line statements. These if statements should include braces around their bodies.
| public sealed class AsyncIntervalGate : IDisposable | ||
| { | ||
| private readonly TimeSpan _interval; | ||
| private readonly SemaphoreSlim _mutex = new(1, 1); | ||
| private DateTime _nextUtc; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="AsyncIntervalGate"/> class. | ||
| /// </summary> | ||
| /// <param name="interval">Minimum interval between permits; negative values are treated as zero.</param> | ||
| public AsyncIntervalGate(TimeSpan interval) | ||
| { | ||
| _interval = interval < TimeSpan.Zero ? TimeSpan.Zero : interval; | ||
| _nextUtc = DateTime.MinValue; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Waits until the next permit is available and then consumes it. | ||
| /// </summary> | ||
| /// <param name="cancellationToken">Token used to cancel the wait.</param> | ||
| public async Task WaitAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| await _mutex.WaitAsync(cancellationToken).ConfigureAwait(false); | ||
| try | ||
| { | ||
| var now = DateTime.UtcNow; | ||
| if (_nextUtc > now) | ||
| { | ||
| var delay = _nextUtc - now; | ||
| if (delay > TimeSpan.Zero) | ||
| { | ||
| await Task.Delay(delay, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| now = DateTime.UtcNow; | ||
| } | ||
|
|
||
| _nextUtc = now + _interval; | ||
| } | ||
| finally | ||
| { | ||
| _mutex.Release(); | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void Dispose() | ||
| { | ||
| _mutex.Dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new AsyncIntervalGate class lacks test coverage. Given that this is a new utility class with complex async logic for rate limiting, it should have unit tests to verify behavior such as proper interval enforcement, cancellation handling, and disposal.
* Added unit tests for `DnsWireMessageParser` to improve coverage. * Refactored existing tests for clarity and maintainability. * Introduced helper methods for reading flags and creating DNS headers. * Ensured proper handling of edge cases in DNS message parsing.
|
Claude finished @PrzemyslawKlys's task —— View job PR Review Complete ✅I've completed a comprehensive review of this PR that adds unit tests for Todo List:
Code Quality & Best Practices ✅Positive aspects:
Areas for improvement:
Potential Issues & Bugs
|
DnsMessageRecursionDesiredTeststo validate the handling of theRecursionDesiredoption in DNS serialization.DnsWireMessageParserTeststo ensure correct parsing of DNS message headers and EDNS records.DnsMessageOptionsto includeRecursionDesiredparameter.DnsMessageto utilize the newRecursionDesiredoption in serialization logic.DnsWireMessageParserfor lightweight parsing of DNS wire-format messages.