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

HTTP fault injection returns 502 (bad gateway) for all requests #292

Closed
pablochacin opened this issue Aug 11, 2023 · 2 comments · Fixed by #300
Closed

HTTP fault injection returns 502 (bad gateway) for all requests #292

pablochacin opened this issue Aug 11, 2023 · 2 comments · Fixed by #300
Assignees
Labels
bug Something isn't working

Comments

@pablochacin
Copy link
Collaborator

pablochacin commented Aug 11, 2023

After changes introduced in #271 all HTTP requests forwarded by the agent return a 502 Bad Gateway error.

@roobre
Copy link
Collaborator

roobre commented Aug 14, 2023

It seems like this was caused by a dumb typo: upstreamReq was cloned and its RequestURI cleared, but req was passed instead of upstreamReq 🤦🏻‍♀️

image

Unit tests did not catch this mistake because the mocked HTTP client never exercises this request. This is the kind of subtlety that integration tests are good at (re. #183)

I am not sure why e2e tests did not catch this.

@pablochacin
Copy link
Collaborator Author

pablochacin commented Aug 15, 2023

Unit tests did not catch this mistake because the mocked HTTP client never exercises this request. This is the kind of subtlety that integration tests are good at (re. #183)
I am not sure why e2e tests did not catch this.

We already have #183 for this discussion about integration tests, but for me, the solution to "e2e test did not catch it" is not to create integration tests because the e2e tests use an actual HTTP server, so I don't see what value it would add.

I think the problem is much simpler: we in the e2e tests are not checking that we don't introduce errors. We are just checking if we get the error we ask for.

So, we don't need to add more tests, just one test case to the existing e2e tests. For example, with errorRate: 0 (do not inject errors) and checking if we get a 200. I suggest we do it in the context of fixing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants