Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert against result object instead of recorder #609

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Assert against result object instead of recorder #609

merged 1 commit into from
Nov 13, 2024

Conversation

jotaen
Copy link
Contributor

@jotaen jotaen commented Nov 13, 2024

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.

Copy link

github-actions bot commented Nov 13, 2024

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

@jotaen
Copy link
Contributor Author

jotaen commented Nov 13, 2024

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

@jotaen
Copy link
Contributor Author

jotaen commented Nov 13, 2024

recheck

github-actions bot added a commit that referenced this pull request Nov 13, 2024
@mtlynch
Copy link
Owner

mtlynch commented Nov 13, 2024

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. 😂

@mtlynch mtlynch merged commit def2dde into mtlynch:master Nov 13, 2024
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2024
@jotaen jotaen deleted the fix-test-setup branch November 13, 2024 16:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants