Issue #3571: Use bugbug to classify issues #3573
Conversation
|
r? @karlcow |
tests/unit/test_webhook.py
Outdated
| @@ -455,6 +455,13 @@ def test_prepare_rejected_issue(self): | |||
| self.assertEqual(type(actual), dict) | |||
| self.assertEqual(actual, expected) | |||
|
|
|||
| @patch('webcompat.webhooks.helpers.make_classification_request') | |||
| def test_get_issue_classification(self, mock_classification): | |||
| """Test that api is called only once with response code is 200.""" | |||
There was a problem hiding this comment.
What is happening if it's called more than once?
Does it fail? Can't happen?
There was a problem hiding this comment.
It's making sure that if we receive a 200 status, make_classification_request is not called again.
So basically classification in bugbug is asynchronous, the first request submits an issue for classification (in response we get 202 as the result is not ready yet) and we need to make a second one to get the results (and we get 200).
This needs another test to test the above ^, but I'm not sure how to do it with a mock:
1 request -> 202
2 request -> 200
assert that mock_classification called twice
tests/unit/test_webhook_model.py
Outdated
| @@ -76,7 +76,7 @@ def test_model_instance(): | |||
| @patch('webcompat.webhooks.model.make_request') | |||
| def test_close_private_issue(mock_mr): | |||
| """Test issue state and API request that is sent to GitHub.""" | |||
| mock_mr.return_value.status_code == 200 | |||
| mock_mr.return_value.status_code = 200 | |||
There was a problem hiding this comment.
So good to have caught this. Now I'm worried… because it doesn't change anything. I wonder what was the value then and how it would modify the test or not. Let's see.
Old:
tests/unit/test_webhook_model.py::test_close_private_issue <MagicMock name='make_request().status_code' id='4363956624'>
PASSED
New:
tests/unit/test_webhook_model.py::test_close_private_issue 200
PASSED
ok definitely wrong.
After removing them, everything is still passing.
So we don't test the status code in any ways.
I think we can safely remove all instances of mock_mr.return_value.status_code = 200
There was a problem hiding this comment.
Yeah I was thinking that too :) I'll remove them
webcompat/webhooks/helpers.py
Outdated
| @@ -230,3 +234,35 @@ def prepare_rejected_issue(): | |||
| payload_request['state'] = 'closed' | |||
| payload_request['milestone'] = invalid_id | |||
| return payload_request | |||
|
|
|||
|
|
|||
| def make_classification_request(issue_number): | |||
There was a problem hiding this comment.
I wonder if we should put the classification functions in a new file: webcompat/webhooks/ml.py?
Just in case in the future, it grows with more functions so we do not end up with a helpers being huge. Maybe it doesn't matter. Opinions?
There was a problem hiding this comment.
Yeah, probably a good idea, I'll move it
webcompat/webhooks/helpers.py
Outdated
| @@ -230,3 +234,35 @@ def prepare_rejected_issue(): | |||
| payload_request['state'] = 'closed' | |||
| payload_request['milestone'] = invalid_id | |||
| return payload_request | |||
|
|
|||
|
|
|||
| def make_classification_request(issue_number): | |||
webcompat/webhooks/helpers.py
Outdated
| if response.status_code == 202: | ||
| time.sleep(retry_sleep) | ||
| else: | ||
| break |
There was a problem hiding this comment.
202 code in this code is a wait instruction. aka the response is not ready.
200 and 4** and 500 will be treated the same.
Is it what we want?
There was a problem hiding this comment.
Yeah, this code will basically keep retrying if the status is 202 up to a certain limit. If it's 200, 4** or 500, we don't need to retry. And in case of 4** or 500 the error will be raised in make_classification_request
webcompat/webhooks/helpers.py
Outdated
| else: | ||
| total_sleep = retry_count * retry_sleep | ||
| msg = f"Couldn't classify issue {issue_number} in {total_sleep} seconds, aborting" # noqa | ||
| raise Exception(msg) |
There was a problem hiding this comment.
we need a test for this, aka when there's a failure.
There was a problem hiding this comment.
Added a test in test_webhook.py 👍
|
I've pushed the fixes, could you please take another look @karlcow? |
|
Thanks! |
This PR fixes issue #3571