Conversation
|
Daniel Moszczanski seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| assert_eq!(response.answers[0].ttl, custom_ttl); | ||
| } | ||
|
|
||
| /// Test that `encode_name` correctly encodes various domain names into DNS wire format |
There was a problem hiding this comment.
In general good idea 👍
Just a note that encode_name() is only used with const domain, and is not for any user facing input. In theory it could have been just harcoded to return [ 4u8, b'm', b'e', b's', b'h', 7, b'n', b'o', b'r', b'd', b's', b'e', b'c', 3, b'c', b'o', b'm', 0 ]; That's why it was not so "thoroughly" tested.
There was a problem hiding this comment.
If it will only ever return that specific enoded name, I vote we hardcode it and get rid of the function
There was a problem hiding this comment.
I agree. But then we have to check twice hardcoded answer.
And then whole PR will be dropped.
|
I think it's a good idea to add more coverage but I would suggest we structure it a little bit differently. First of all, let's place the different cases in separate tests so each test has one clear responsibility. Multiple cases that tests the same thing can be in the same test. And then I would like to place all tests for |
4 participants
Problem
The packet_encoder module in telio-dns contained a general-purpose encode_name function for serialising DNS domain names into wire format. However, the function is only ever called with two fixed compile-time constants (SOA_MNAME and SOA_RNAME), so its runtime validation logic (empty name, empty label, label too long, name too long) can never be triggered in production. The existing test suite reflected this mismatch: all tests for encode_name lived in a flat mod tests block alongside unrelated DnsResponseBuilder tests, making the structure hard to navigate and the responsibility of each test unclear.
Solution
Two changes were made in crates/telio-dns/src/packet_encoder.rs:
1 - Test restructuring - All tests for encode_name were moved into a dedicated nested mod encode_name block inside mod tests, giving them a single clear scope. Test names were shortened by dropping the redundant encode_name_ prefix (e.g. encode_name_empty_fails - empty_name_fails). Each test retains one clear responsibility; tests that cover two values of the same concept (e.g. recursion_desired_set) are kept as a single test. No logic was changed - all 52 tests continue to pass.
2 - Binary correctness test - A new test binary_correctness was added inside mod encode_name that verifies encode_name produces the correct DNS wire-format bytes for a representative set of inputs: a multi-label domain, an FQDN with a trailing dot, a single-label name, a label at the maximum allowed size (63 bytes), a name at the maximum allowed total size (255 bytes encoded), and a name with leading/trailing whitespace that should be trimmed. This test documents and guards the exact byte-level output of the function, which is the format consumed directly by the SOA RDATA builder.
☑️ Definition of Done checklist