mtlynch / picoshare

A minimalist, easy-to-host service for sharing images and other files
https://demo.pico.rocks
Other
2.34k stars 169 forks source link

Assert against result object instead of recorder #609

Closed jotaen closed 1 week ago

jotaen commented 2 weeks ago

I noticed that there is a subtle issue in the test setup, which could potentially lead to false positive test results.

According to the documentation of httptest.ResponseRecorder (see their example), you are supposed to run the test assertions against the result object returned by recorder.Result(), rather than against the recorder object itself.

The recorder object basically tracks all interactions of the request handlers with the response. However, this may be different from what the HTTP server actually sends out to the client.

One such edge-case scenario is when you try to set response headers after having flushed out the response headers. In this case, any attempt to modify the headers via res.Headers().Set() will silently result in a no-op. The response object issued by rec.Result() correctly reflects that, whereas the recorder object itself incorrectly yields the seemingly set header.

github-actions[bot] commented 2 weeks ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

jotaen commented 2 weeks ago

I have read the CLA Document and I hereby sign the CLA

jotaen commented 2 weeks ago

recheck

mtlynch commented 1 week ago

Thanks, @jotaen!

This is another situation where I'm glad to learn this gotcha but sad thinking about the work ahead of me in correcting this mistake in the 5000 places I've made it in other projects. 😂