Canonical json form for signed responses #3035
Conversation
michellemounde
changed the title
There was a problem hiding this comment.
I know this is still just a draft, but I just want to note that we'll want one or more tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/web/test_json.py.
Also, if you need a hand with updating the requirements please let me know. Running maintenance/pin.sh should do it though.
michellemounde
force-pushed
the
canonical-json-responses
branch
from
3155b23 to
2b6a725
Compare
|
@bhearsum I have a question. The canonical JSON tests are passing but they are also passing when I just use JSON. Should I also test for the signature in this case? I am not sure if I should do this or if it would just give the same result. I tried mocking a unique signature for the test cases but I still got the same signature because I don't know how to pass a non-null content to: balrog/src/auslib/web/public/helpers.py Line 30 in a4536ab |
There was a problem hiding this comment.
@bhearsum I have a question. The canonical JSON tests are passing but they are also passing when I just use JSON.
I left a comment below about this.
Should I also test for the signature in this case? I am not sure if I should do this or if it would just give the same result.
No - don't worry about the signature or any other headers here. It's out of scope for these new tests.
tests/web/test_json.py
Outdated
| ) | ||
| def testGuardianResponseWithCanonicalJson(client, version, buildTarget, channel, code, response): | ||
| ret1 = client.get(f"/json/1/Guardian/{version}/{buildTarget}/{channel}/update.json") | ||
| ret2 = client.get(f"/json/1/Guardian/{version}/{buildTarget}/{channel}/update.json") |
There was a problem hiding this comment.
I'm not entirely certain what you're testing here. You seem to make making the same request twice, and asserting that the results are the same (which I would assume they would be...).
I think what you should be doing is comparing a string version of the JSON with a string that is hardcoded in the expected, canonical order.
There was a problem hiding this comment.
I have made the requested change.
michellemounde
requested review from
jcristau,
gbrownmozilla and
gabrielBusta
as code owners
tests/web/test_json.py
Outdated
| assert ret.status_code == code | ||
| if code == 200: | ||
| assert ret.mimetype == "application/json" | ||
| assert ret.get_json() == response |
There was a problem hiding this comment.
This (and the response entries it uses) are unnecessary. Comparing the raw text to the expected JSON is all we need to do here.
(Calling get_json is just re-parsing the raw text into a dictionary - so we're effectively checking the expected result twice.)
There was a problem hiding this comment.
Updated this as requested
|
I tried rebasing and resolving merge conflicts but I got an error when running |
Hm...it looks like this is a general problem unrelated to this patch. I'll try to put together a fix for it. |
Once #3050 is merged you should be able to rebase and update the requirements. |
michellemounde
force-pushed
the
canonical-json-responses
branch
from
47a1fdf to
70bfd23
Compare
Fixes #1112