Fix various tests #236
Fix various tests #236
Conversation
|
So the @Mr-Sunglasses are those failing tests designed to be run manually after deploying some test version of the app to Heroku? If that's the case I can exclude them from the GHA test command. Otherwise, if they are meant to run during CI on every PR, what would the desired fix look like? |
| @@ -28,8 +28,14 @@ def query_request(query=None, method="GET", **kwargs): | |||
| "headers": {"Authorization": f"Bearer {GITHUB_TOKEN}"} | |||
| } | |||
|
|
|||
| for kw in kwargs: | |||
| request_kwargs["headers"].update(kwargs[kw]) | |||
There was a problem hiding this comment.
The old code had a bug where non-header parameters were being placed under the headers key, instead of at the top level
e.g.
# Incorrect
{
"headers": {
"a": 1,
"json": {...},
},
}
vs.
# Correct
{
"headers": {"a": 1},
"json": {...},
}
| @@ -62,7 +68,9 @@ def update_dict(base, head): | |||
|
|
|||
| def match_webhook_secret(request): | |||
| """Match the webhook secret sent from GitHub""" | |||
| if os.environ.get("OVER_HEROKU", False): | |||
There was a problem hiding this comment.
This bug was due to the fact that the string "False" actually evaluates as True when tested as a boolean, only the empty string "" gives a False value.
Fixed by accounting for all varieties of a "True" env var.
| @@ -20,7 +20,6 @@ def test_request(self, mocker, query, method, json, data, headers, params): | |||
| assert mock_func.call_count == 1 | |||
| assert mock_func.call_args[0][0] == method | |||
| assert mock_func.call_args[1]['headers'] == headers | |||
| assert mock_func.call_args[1]['auth'] == (os.environ['BOT_USERNAME'], os.environ['GITHUB_TOKEN']) | |||
There was a problem hiding this comment.
Fixed to use the current authentication method.
| @@ -45,43 +44,45 @@ def test_request(self, mocker, query, method, json, data, headers, params): | |||
| def test_update_dict(self, base, head, expected): | |||
| assert update_dict(base, head) == expected | |||
|
|
|||
| def test_match_webhook_secret(self, monkeypatch, request_ctx): | |||
There was a problem hiding this comment.
request_ctx no longer exists, fixed by using a MagicMock instead.
| headers={"X-GitHub-Event": event}, | ||
| # TODO: This test is not representative of the real JSON payloads sent by Github and should be updated at | ||
| # some point to have a sample payload fixture for each of the above types. | ||
| json={"a": "value", "b": 1}, |
There was a problem hiding this comment.
This json param was required, but missing. Ideally the value would look more like a real payload, but those are hundreds/thousands of lines long and I didn't have a good example to dump in here.
|
Hey @Mr-Sunglasses, just wondering if you'll have a chance to review this over the next few days? |
Description
This PR fixes the second half of #194.
During #234, the tests were enabled on GHAs but they weren't passing.
Previous test result from
master:Here I've made a few small tweaks to get the test suite passing locally at least (will have to see what happens on GHA):
I will try to comment on each fix in-line.