testing: add more unit tests to increase branch coverage

[mohammed90]

Self-expalantory. We need to add more tests; and I wondered if LLMs could generate tests covering all the logic branches, so this is the result.

Assistance Disclosure

I used Amp to generate the tests, tweaked few by hand, and fixed the bug by hand. The generated code will not be taken and accepted at face value. Changes to the source code (not tests) by the agent will not be accepted. Only the tests cases with study of coverage.

Prompt:

Study the full codebase of the project. Write unit tests to validate the functionality and ensure robustness. The tests should check for the intended behavior, not the behavior as written. It is possible the existing code base has an unknown defect, for which we need the unit tests to uncover. Write the unit tests.

[mohammed90]

AI disclosure: copilot + claude opus

Bugs Found

Bug 1: importGraph.addEdge Does Not Detect Self-Loops

File: caddyconfig/caddyfile/importgraph.go, line 60
Test: caddyconfig/caddyfile/importgraph_test.go → TestAddEdgeSelfLoop
Severity: Medium — could allow a Caddyfile to import itself, causing infinite recursion.

Description:
When addEdge("a", "a") is called, the willCycle(to, from) check fails to detect the self-loop. The willCycle function traverses existing edges from to to see if from is reachable, but since the edge hasn't been added yet, the DFS traversal finds nothing.

Root Cause:
willCycle does not check the trivial case where from == to before traversing edges.

Suggested Fix:

func (i *importGraph) addEdge(from, to string) error {
    // ...
    if from == to {
        return fmt.Errorf("a cycle of imports exists between %s and %s", from, to)
    }
    if i.willCycle(to, from) {
    // ...

---

Bug 2: importGraph.removeNode Does Not Clean Up Edges

File: caddyconfig/caddyfile/importgraph.go, line 48
Test: caddyconfig/caddyfile/importgraph_test.go → TestRemoveNodeCleansUpEdges
Severity: Low — orphaned edges remain in the adjacency map after a node is removed.

Description:
removeNode(name) only deletes the node from i.nodes but does not remove:

1. Outgoing edges from the removed node (i.edges[name])
2. Incoming edges to the removed node (references in other nodes' edge lists)

This leaves stale data in the edge map, which could cause incorrect cycle detection if the node name is later reused.

Suggested Fix:

func (i *importGraph) removeNode(name string) {
    delete(i.nodes, name)
    delete(i.edges, name) // remove outgoing edges
    for k, v := range i.edges { // remove incoming edges
        i.edges[k] = slices.DeleteFunc(v, func(s string) bool { return s == name })
    }
}

---

Bug 3: SetVar Stores nil for Non-Existent Keys Instead of Being a No-Op

File: modules/caddyhttp/vars.go, line 460
Test: modules/caddyhttp/marshalers_test.go → TestSetVarNilDeletesKey
Severity: Low — unexpected nil entries accumulate in the vars map.

Description:
When SetVar(ctx, key, nil) is called for a key that does not exist in the map, the intended behavior appears to be a no-op (the nil-value branch only handles deletion of existing keys). However, execution falls through to varMap[key] = value on line 471, which inserts a nil entry into the map.

Code Flow:

func SetVar(ctx context.Context, key string, value any) {
    varMap, ok := ctx.Value(VarsCtxKey).(map[string]any)
    if !ok { return }
    if value == nil {
        if _, ok := varMap[key]; ok {
            delete(varMap, key)
            return   // ← only returns if key exists
        }
        // ← falls through here if key doesn't exist
    }
    varMap[key] = value  // ← stores nil for non-existent key
}

Suggested Fix:

if value == nil {
    delete(varMap, key) // delete is a no-op for non-existent keys
    return
}

---

Bug 4: MaxSizeSubjectsListForLog Leaks One Item When maxToDisplay=0

File: internal/logs.go, line 9
Test: internal/ranges_test.go → TestMaxSizeSubjectsListForLog/maxToDisplay_zero
Severity: Low — when called with maxToDisplay=0, one domain leaks into the output.

Description:
When maxToDisplay=0 and the subjects map is non-empty, numberOfNamesToDisplay is correctly computed as min(len(subjects), 0) = 0. However, the loop body appends one item before checking the bound:

for domain := range subjects {
    domainsToDisplay = append(domainsToDisplay, domain)  // ← appends first
    if len(domainsToDisplay) >= numberOfNamesToDisplay {  // ← 1 >= 0 is true
        break
    }
}

The result is a slice containing one domain plus the suffix "(and N more...)" instead of just the suffix.

Suggested Fix:
Check the bound before appending, or handle maxToDisplay <= 0 as a special case:

for domain := range subjects {
    if len(domainsToDisplay) >= numberOfNamesToDisplay {
        break
    }
    domainsToDisplay = append(domainsToDisplay, domain)
}

[francislavoie]

I guess we should figure out the official spec on Go's package string format, I figure an @ immediately after a / would just be eaten as part of the package name, not count as a version unless there's something alpha before the @ 🤔

[francislavoie]

Lmao what?

[francislavoie]

Oh 🤣 I didn't realize we actually had a func reverse, I assumed this was testing a Go built-in

[francislavoie]

LoggableHTTPRequest will probably be moved to internal in #7578

[mohammed90]

I have it on my radar

[steadytao]

I have it on my radar

It has been moved over 🙂