Skip to content

test: restore the old test 1609 as 3271 - #23052

Closed
bagder wants to merge 2 commits into
masterfrom
bagder/1609-3271
Closed

bagder wants to merge 2 commits into
masterfrom
bagder/1609-3271

Conversation

@bagder

@bagder bagder commented Sep 23, 2026

Copy link
Copy Markdown
Member

It was wrongly removed as a duplicate but the code remained in tests/unit and was never exercised.

The internal dns cache logic and API changed in the meantime so test is also updated to reflect those.

Follow-up to 4ead428

It was wrongly removed as a duplicate but the code remained in
tests/unit and was never exercised.

The internal dns cache logic and API changed in the meantime so test is
also updated to reflect those.

Follow-up to 4ead428
@bagder bagder added tests name lookup DNS and related tech labels Sep 23, 2026
@bagder
bagder requested review from icing and a balanced review from Copilot September 23, 2026 09:30
@bagder
bagder marked this pull request as ready for review September 23, 2026 09:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The test recreates its DNS cache each iteration, so it never exercises cache-entry replacement or permanence conversion.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Restores test 1609 as unit test 3271 and adapts it to the current DNS cache key API.

Changes:

  • Registers and renames the restored unit test.
  • Uses peer-based DNS cache keys.
  • Exposes internal key helpers for unit testing.
File Description
tests/​unit/​unit3271.c Restores and updates the resolve override test.
tests/​unit/​Makefile.inc Registers unit3271.
tests/​data/​test3271 Defines the test case metadata.
tests/​data/​Makefile.am Adds test3271 to the suite.
lib/​vdns/​dnscache.h Exposes DNS key structures.
lib/​vdns/​dnscache.c Makes key helpers unit-testable.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/vdns/dnscache.c Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@testclutch

Copy link
Copy Markdown

Analysis of PR #23052 at 8bf2dae5:

Test 1041 failed, but it has been 0.7% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 12 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Generated by Testclutch

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The test recreates its DNS cache between cases and does not verify the intended permanence behavior.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reuse one multi handle and DNS cache across iterations

tests/​unit/​unit3271.c:102

This still creates and destroys multi inside every iteration (lines 121 and 204). Each multi owns its DNS cache, so the second case never sees the first entry and therefore tests two independent insertions rather than the advertised override. Keep one multi/cache alive across both iterations (removing each easy handle before cleanup), then destroy it after the loop.

Medium severity Verify CURLOPT_RESOLVE replacement changes cache entry permanence

tests/​unit/​unit3271.c:141

The retrieved cache entry is only checked for its address and port, so the stated expiry/permanence regression can pass even if CURLOPT_RESOLVE leaves the replacement non-permanent. Seed the first shared-cache entry as non-permanent (using the + form), then assert dns->permanent is false before the replacement and true afterward.

@bagder bagder closed this in 8807773 Sep 23, 2026
@bagder
bagder deleted the bagder/1609-3271 branch September 23, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

name lookup DNS and related tech tests

Development

Successfully merging this pull request may close these issues.

3 participants