Skip to content
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.

Sign up for GitHub

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

Fix various tests #236

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix various tests #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
487dc59
Fix various parts of the unit tests to get them passing on GHAs
kyluca Oct 16, 2024
997d9a6
Fix OVER_HEROKU test logic
kyluca Oct 16, 2024
11f7bcb
Fix OVER_HEROKU test logic
kyluca Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions pep8speaks/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for kw, value in kwargs.items():
# Merge the headers dicts instead of overwriting it
if kw in request_kwargs:
request_kwargs[kw].update(value)

else:
request_kwargs[kw] = value

return requests.request(method, query, **request_kwargs)


Expand Down Expand Up @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

heroku_flag = os.environ.get("OVER_HEROKU", False)

if heroku_flag in (True, "True", "TRUE"):
if ('X-Hub-Signature' in request.headers and
request.headers.get('X-Hub-Signature') is not None):
header_signature = request.headers.get('X-Hub-Signature', None)
Expand Down
33 changes: 17 additions & 16 deletions tests/local/pep8speaks/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@

class TestUtils:
@pytest.mark.parametrize('query, method, json, data, headers, params', [
('/someurl', 'POST', {'k1': 'v1'}, '', None, None),
('http://someurl.com', 'GET', None, '', 'h1=v1', 'k1=v1'),
('/someurl', 'POST', {'k1': 'v1'}, '', {'Authorization': 'Bearer '}, None),
('http://someurl.com', 'GET', None, '', {'Authorization': 'Bearer ', 'h1': 'v1'}, 'k1=v1'),
])
def test_request(self, mocker, query, method, json, data, headers, params):
mock_func = mock.MagicMock(return_value=True)
Expand All @@ -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'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to use the current authentication method.

assert mock_func.call_args[1]['params'] == params
assert mock_func.call_args[1]['json'] == json
if query[0] == "/":
Expand All @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request_ctx no longer exists, fixed by using a MagicMock instead.

assert match_webhook_secret(request_ctx) is True
def test_match_webhook_secret(self, monkeypatch):
mock_request = mock.MagicMock()

monkeypatch.setenv('OVER_HEROKU', False)
assert match_webhook_secret(mock_request) is True

request_ctx.headers = {'Header1': True}
monkeypatch.setenv('OVER_HEROKU', 'True')

mock_request.headers = {'Header1': True}
with pytest.raises(werkzeug.exceptions.Forbidden):
match_webhook_secret(request_ctx)
match_webhook_secret(mock_request)

request_ctx.headers = {'X-Hub-Signature': None}
mock_request.headers = {'X-Hub-Signature': None}
with pytest.raises(werkzeug.exceptions.Forbidden):
match_webhook_secret(request_ctx)
match_webhook_secret(mock_request)

key, data = 'testkey', 'testdata'

hmac_obj = hmac.new(key.encode(),
data.encode(),
digestmod="sha1")

request_ctx.headers = {
mock_request.headers = {
'X-Hub-Signature': f'{hmac_obj.name}={hmac_obj.hexdigest()}'
}
with pytest.raises(werkzeug.exceptions.NotImplemented):
match_webhook_secret(request_ctx)
match_webhook_secret(mock_request)

hmac_obj = hmac.new(key.encode(),
data.encode(),
digestmod="sha1")

request_ctx.headers = {
mock_request.headers = {
'X-Hub-Signature': f'sha1={hmac_obj.hexdigest()}'
}
request_ctx.data = data.encode()
mock_request.data = data.encode()

monkeypatch.setenv('GITHUB_PAYLOAD_SECRET', 'wrongkey')
with pytest.raises(werkzeug.exceptions.Forbidden):
match_webhook_secret(request_ctx)
match_webhook_secret(mock_request)

monkeypatch.setenv('GITHUB_PAYLOAD_SECRET', key)
assert match_webhook_secret(request_ctx) is True
assert match_webhook_secret(mock_request) is True
9 changes: 7 additions & 2 deletions tests/local/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ def test_main_post(self, mocker, client, event, action):
mock_func = mock.MagicMock(return_value=True)
mocker.patch('pep8speaks.utils.match_webhook_secret', mock_func)
mocker.patch('pep8speaks.handlers.' + action, mock_func)
client.post(url_for('main'),
headers={"X-GitHub-Event": event})
client.post(
url_for('main'),
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},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
assert mock_func.call_count == 2
Loading