Skip to content

test: hosts, routes, serve coverage and URL integration#17

Merged
GingerAdonis merged 1 commit intomainfrom
superset/repeated-leaf
Apr 9, 2026
Merged

test: hosts, routes, serve coverage and URL integration#17
GingerAdonis merged 1 commit intomainfrom
superset/repeated-leaf

Conversation

@GingerAdonis
Copy link
Copy Markdown
Contributor

Adds unit tests for hostname resolution and daemon info lookup, a runRoutes table test with a stub Unix socket, and resolveServeSocketPath cases (invalid config, config socket vs flag).

Includes an integration test that runs hostmux url against a foreground daemon so daemon-domain expansion is exercised end-to-end. Updates the existing domain-inheritance integration test to use run --name api, matching the current CLI.

Verification

  • go test ./... -count=1
  • go test -tags=integration -count=1 ./...
  • go vet ./...

Made with Cursor

- Unit-test resolveRequestedHosts and daemon info lookup over net.Pipe
- Exercise runRoutes against a minimal Unix socket stub
- Cover resolveServeSocketPath invalid config and socket precedence
- Add integration test for url inheriting daemon domain; fix run test for --name

Made-with: Cursor
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@opencode-agent
Copy link
Copy Markdown

opencode-agent bot commented Apr 9, 2026

Review Summary

This PR adds good test coverage for hostname resolution, daemon info lookup, and socket path resolution. All tests pass and code is properly formatted.

Assessment: LGTM

Changes Reviewed

  1. hosts_test.go (new file)- Well-structured tests for resolveRequestedHosts covering domain expansion, trailing dot stripping, prefix handling, and full hostname preservation
  • Daemon info/domain lookup tests using net.Pipe() for synchronous mock socket communication
  1. routes_test.go (new file)- Single test for runRoutes output format using a Unix socket stub

  2. serve_test.go (new file)- Tests for resolveServeSocketPath covering invalid config, config socket, and flag override precedence

  • Proper environment isolation with t.Setenv
  1. integration_test.go (modified)
  • Fix: run apirun --name api to match current CLI
  • New TestURLInheritsDomainFromDaemonConfig validates daemon domain inheritance for the url command

Verification

  • ✅ All unit tests pass
  • ✅ All integration tests pass
  • go vet clean
  • ✅ Code properly formatted

New%20session%20-%202026-04-09T14%3A05%3A31.083Z
opencode session  |  github run

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Good set of tests. All unit tests pass. A few observations:

--name fix in TestRunInheritsDomainFromDaemonConfig

Necessary correctness fix — the old run api positional-arg form no longer matches the CLI. ✓


serve_test.go

Env isolation is correct: clearing HOME, HOSTMUX_SOCKET, and XDG_RUNTIME_DIR ensures sockpath.ResolveServe uses only Flag/ConfigSocket in the tests that depend on it. ✓


routes_test.go

The stub goroutine handles a single connection and exits. Since Resolve short-circuits on a non-empty Flag, HOSTMUX_SOCKET is never consulted, so no env isolation is needed here. ✓

One minor nit: the stub swallows errors silently (return on decode failure). If the stub misbehaved, the test would fail on the runRoutes side with a confusing error instead of a clear message. Consider logging t.Errorf in the goroutine for easier debugging:

go func() {
    conn, err := ln.Accept()
    if err != nil {
        t.Errorf("stub accept: %v", err)
        return
    }
    ...
}()

hosts_test.go

Tests cover the key branching in lookupDaemonInfoClient: success, error message propagation, rejected without error field, and PublicHTTPS false case. Assertions are tight and match the production code. ✓

One nit: TestLookupDaemonInfoClient_PropagatesDaemonError only checks strings.Contains(err.Error(), "refused"). It would more precisely verify error propagation if it also checked for the "daemon rejected info lookup" prefix, but this is minor.


integration_test.goTestURLInheritsDomainFromDaemonConfig

Correctly structured and consistent with existing integration tests. Uses waitForSocket rather than waitForTCP, which is sufficient since hostmux url only uses the Unix socket and that socket is created after TLS setup completes in serve.go. ✓

Style nit: The binary-build block (lines 249–253) is now copy-pasted three times across the integration test file. Worth extracting into a buildTestBinary(t *testing.T) string helper to reduce duplication — not a blocker, but worth doing before a fourth test appears.


Overall this is solid work. No bugs or correctness issues found.

@GingerAdonis GingerAdonis merged commit 9792741 into main Apr 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant