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? |
| } | ||
|
|
||
| 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": {...},
}
|
|
||
| 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.
| 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.
| 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? |
1 participant
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.