Conversation
|
I think this looks good and illustrates the importance of writing the correct tests. |
aagbsn
left a comment
aagbsn
left a comment
There was a problem hiding this comment.
This looks good; thanks for catching these mistakes. The FastAPI post handling for Optional fields is a bit confusing to me though; how does the debug_prioritization endoint not complain about a missing parameters without Optional?
| ) | ||
| def debug_prioritization( | ||
| clickhouse: ClickhouseDep, | ||
| probe_cc: Optional[str] = Query(description="2-letter Country-Code", default="ZZ"), |
There was a problem hiding this comment.
I don't understand why the test added is returning status 200 if non-optional parameters are not supplied
There was a problem hiding this comment.
Because if it's set to Optional (i.e. nullable) with a default value set, effectively it's never possible for it to assume the None value, hence the correct typing should be str
There was a problem hiding this comment.
ok this is a bit confusing: as a URL parameter; if it is NOT set to optional but has a default value set, it effectively is optional?
There was a problem hiding this comment.
Something like that. The thing is that there is no way to specify using HTTP semantics that this field which has a default value should actually be none
| assert resp.status_code == 200 | ||
| assert resp.headers["content-type"] == "application/json" | ||
|
|
||
| resp = client.get("/api/_/debug_prioritization") |
There was a problem hiding this comment.
see comment above; probe_cc isn't supplied in this test - so the Query parameter is effectively optional if a default value is supplied? That's why I'm surprised this test passes, because I thought Optional[] was used to indicate whether the parameter was required or not.
There was a problem hiding this comment.
The thing is that all values have now defaults set, which is the behaviour we had in the previous version of it. So now because there are always defaults, the call to generate_test_list is successful, while previously it failed when probe_asn was none
2 participants
This addresses the diffs in this endpoint identified in the test deployment here: ooni/devops#300