-
Notifications
You must be signed in to change notification settings - Fork 32
add bouncer/net-tests to ooniapi under new router bouncer #1036
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
base: master
Are you sure you want to change the base?
Conversation
| assert "net-tests" in j | ||
| for v in j["net-tests"]: | ||
| for x in ["collector", "input-hashes", "name", "test-helpers", "test-helpers-alternate", "version"]: | ||
| assert x in v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also port the other tests that are here:
| def test_bouncer_net_tests(client): |
Mostly to be sure we're not breaking the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the other tests - thank you for pointing those out - but I changed the expected response code because FastAPI returns error 422 rather than errorcode 400. Legacy probes may or may not check for a specific errorcode so we need to confirm that before merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather go with the safe route in this one, let's change it to 400.
I was looking into how this can be done and it seems like parsing the request manually is the way to go in this case, sorry for asking you to change it in another comment
We could do something like
try:
j = await req.json()
m = NetTestsRequest(**j)
except pydantic.ValidationError as e:
raise HTTPException(400, detail=e.errors())
(or you could manually parse the json like you had before, any choice is good to me)
It would mess the api docs for this endpoint but in this case it doesn't really matter since this endpoint is not used by new clients and we keep it just for backwards compatibility
|
related to #807 |
I modified the tests to expect any error (other than http response 200) because FastAPI returns HTTP error 422 Unprocessable Entity instead of 400, as the tests do not supply a request that the model can validate
No description provided.