Skip to content

test: rename handlers/*.test.go so the tests actually run - #154

Open
manantlerio wants to merge 1 commit into
everywall:mainfrom
manantlerio:fix/test-files-never-run
Open

manantlerio wants to merge 1 commit into
everywall:mainfrom
manantlerio:fix/test-files-never-run

Conversation

@manantlerio

Copy link
Copy Markdown

Summary

handlers/api.test.go, handlers/proxy.test.go and handlers/raw.test.go are
named with a dot rather than an underscore. go test only picks up
*_test.go, so none of these have ever been compiled or run as tests. They are
built into the package as ordinary source, which is also why testing and
net/http/httptest end up as imports of the non-test build.

Renaming them to *_test.go runs them for the first time. All four tests
failed
, so this fixes them as well.

Every failure turned out to be in the test, not in the handler. No runtime
behaviour is changed in this PR.

What was broken

TestProxySite mounted the handler on /:url:

app.Get("/:url", ProxySite(""))

ProxySite reads c.Params("*"), and cmd/main.go mounts it on /*. A single
:url param cannot match https://example.com, so the request 404'd and the
handler was never reached at all. Now mounted on /* to match production.

TestApi / invalid url expected 400. Api returns 500.

TestRaw / invalid url expected status 200 and the body
parse invalid-url: invalid URI for request. Raw returns 500 and
Get "invalid-url": unsupported protocol scheme "", i.e. the error comes from
the HTTP client, not from url.Parse. The test also asserted StatusCode == 200
for every case, so it could not have expressed an error case even in principle.

TestRewriteHtml expected <img src= but rewriteHtml emits <img src=,
with two spaces. The replacement is "<img $1 src=" and $1 is empty when the
tag carries no other attributes.

Judgement calls

Two of these look like real (if minor) defects, and I have deliberately not
fixed them here, because this PR is about test naming and fixing them would
change behaviour:

  1. A malformed target URL returns 500. Arguably it should be 400, since it
    is the caller's input that is wrong. Changing it is an API change. Happy to
    send it separately if you want it.
  2. rewriteHtml emits a double space in <img src= and <script src=.
    Harmless in HTML, but it is sloppy output and the original test author
    clearly expected a single space. Fixing it properly means restructuring the
    capture group so the whitespace lives outside it, which is a change to output
    on every proxied page. Also happy to send separately.

Both are asserted as current behaviour with a comment saying so, so the tests
document reality rather than blessing it silently.

Also

The two "valid url" cases pointed at www.google.com. I switched them to
example.com, which is reserved by IANA for exactly this purpose and is 559
bytes rather than a full search page.

They still require network access, so the suite is not hermetic. Converting them
to httptest servers would fix that and is worth doing, but it interacts with
#153 (which blocks fetches to private addresses, including loopback), so it
wants to be a deliberate follow-up rather than a drive-by here.

Verification

go vet, gofmt, go build ./... clean and go test ./... green, in
golang:1.26:

ok  ladder/handlers      0.085s
ok  ladder/pkg/ruleset   1.134s

Independent of #153, which touches different files.

The three test files in handlers/ are named with a dot rather than an
underscore, so `go test` has never compiled or run any of them. They are built
into the package as ordinary source instead, which is also why the testing and
httptest imports end up in the non-test build.

Renaming them to *_test.go turns them on for the first time. All four tests
failed, so this fixes them too. Every failure is in the test rather than in the
handler, and no runtime behaviour is changed here.

TestProxySite: mounted the handler on "/:url" while ProxySite reads
c.Params("*"), so the route never matched a target URL and the request 404'd
without reaching the handler. Now mounted on "/*", the way cmd/main.go does it.

TestApi/invalid url: expected 400, Api returns 500. TestRaw/invalid url:
expected 200 and a url.Parse error string, Raw returns 500 and the error from
the HTTP client. Both assertions were guesses that had never been executed. The
tests now assert what the handlers do. Whether a bad target URL ought to be 400
rather than 500 is a real question, but changing it is an API change and does
not belong in a PR about test naming.

TestRewriteHtml: expected "<img src=" but rewriteHtml emits "<img  src=". The
replacement is "<img $1 src=" and $1 is empty when the tag has no other
attributes. Harmless in HTML, and asserted as-is rather than quietly corrected
for the same reason as above.

The two "valid url" cases pointed at www.google.com. Switched to example.com,
which is reserved for exactly this and is a great deal smaller. They still
require network access; making the suite hermetic is worth doing separately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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