Add a DNSServer for mocking/tests #53
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new DNSServer module for mocking and testing DNS functionality. It refactors the codebase into a modular architecture with a shared DNSProtocol module, updates the testing framework from XCTest to Swift Testing, and raises the minimum Swift version requirement.
Changes:
- Introduced a new DNSServer module with UDP/TCP support for mocking DNS responses in tests
- Extracted common DNS protocol code into a separate DNSProtocol module shared by client and server
- Migrated all tests from XCTest to the Swift Testing framework with async/await support
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Package.swift | Updated Swift tools version to 6.0, raised platform requirements, added DNSProtocol and DNSServer targets |
| Sources/DNSProtocol/* | New module containing shared DNS protocol types, encoders/decoders, and record structures |
| Sources/DNSServer/* | New DNS server implementation with delegate pattern for handling queries via UDP/TCP |
| Sources/DNSClient/* | Refactored to depend on DNSProtocol module, removed duplicate code |
| Tests/DNSClientTests/DNSServerTests.swift | New comprehensive tests for DNS server functionality |
| Tests/DNSClientTests/DNSUDPClientTests.swift | Migrated to Swift Testing framework with async/await |
| Tests/DNSClientTests/DNSTCPClientTests.swift | Migrated to Swift Testing framework with async/await |
Comments suppressed due to low confidence (1)
Sources/DNSProtocol/ByteBuffer+DNS.swift:300
- The writeCompressedLabels implementation has changed significantly. The comment at line 299 says 'Store position for this label sequence BEFORE writing', but it's storing the position for the full remaining sequence (fullKey) which includes the current label. This logic differs from the removed implementation and should be documented more clearly to explain the compression algorithm.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| let fallbackAddress = try! SocketAddress(ipAddress: "0.0.0.0", port: 0) |
There was a problem hiding this comment.
Using force-try (try!) can cause crashes if SocketAddress initialization fails. While '0.0.0.0:0' is likely valid, it's better practice to handle this gracefully or use a pre-initialized constant to avoid potential runtime crashes in production code.
| let fallbackAddress = try! SocketAddress(ipAddress: "0.0.0.0", port: 0) | |
| let fallbackAddress: SocketAddress | |
| do { | |
| fallbackAddress = try SocketAddress(ipAddress: "0.0.0.0", port: 0) | |
| } catch { | |
| // If we cannot even create a fallback address, close the connection to avoid a crash. | |
| context.close(promise: nil) | |
| return | |
| } |
| // Wait a moment | ||
| try await Task.sleep(nanoseconds: 100_000_000) | ||
|
|
There was a problem hiding this comment.
The hardcoded sleep duration (100ms) is used to wait for connection establishment. This is brittle and may cause flaky tests. Consider using a more deterministic synchronization mechanism or document why this delay is necessary.
| // Wait a moment | |
| try await Task.sleep(nanoseconds: 100_000_000) |
| .macOS(.v13), | ||
| .iOS(.v16), |
There was a problem hiding this comment.
The platform requirements have been raised significantly (from macOS 10.15/iOS 13 to macOS 13/iOS 16). This is a breaking change that may affect users on older platforms. Ensure this is intentional and documented in release notes.
| .macOS(.v13), | |
| .iOS(.v16), | |
| .macOS(.v10_15), | |
| .iOS(.v13), |
No description provided.