Refactor parse_commands to add tests. #149

alexrs · 2018-03-13T15:13:45Z

This PR address #143. This is a first step in making Homu testable.

A small refactor has been done, extracting the logic in parse_commands to its own functions, to be able to mock the state and add tests.

This change is

jdm

Solid effort to test the behaviour of the particular handlers! What's the plan for getting parse_commands under test?

jdm · 2018-03-13T16:49:38Z

tests/test_main.py

+        self.assertFalse(sha_cmp('f25', 'f259660b128ae59133dff123998ee9b643aff050'))
+
+if __name__ == '__main__':
+    unittest.main()


Let's run these tests on TravisCI by modifying .travis.yml.

It seems to be failing with Python 3.5 https://travis-ci.org/servo/homu/jobs/353098303#L696

What if you update https://github.com/servo/homu/blob/master/setup.py#L13 to only allow versions that are <1.0? That was released yesterday and significantly changes the library, so we probably don't want to blindly allow it; it also introduced the dateutil import that's breaking here.

After some research, I just discovered why some tests are failing. Apparently, the method assert_called_once was introduced in Python 3.6. In Python 3.5 any method starting assert_ but not implemented seems to raise an AttributeError but before Python 3.5, we're just calling a mocked method like any other (that doesn't exist in this case, but it doesn't matter). Any suggestion for a replacement of assert_called_once?

Can you use assert_called_once_with instead?

Yeah, it works now! 🎉

jdm · 2018-03-13T16:52:35Z

tests/test_main.py

+    def test_sha_or_blank_return_blank(self):
+        self.assertEqual(sha_or_blank('f5d@12'), '')
+
+    def test_sha_cpm_equal(self):


cmp, not cpm

jdm · 2018-03-13T16:53:49Z

homu/main.py

+    # TODO: We don't need partials here.
+    # I guess that we can use partials, as the
+    # only parameter that changes is the AuthState.
+    # We need to refactor this.


It's not clear what this comment adds to the code.

jdm · 2018-03-13T17:26:23Z

homu/main.py

+    state.save()
+
+
+def treeclosed_equal(state, word):


Let's call this set_treeclosed

jdm · 2018-03-13T17:30:02Z

homu/main.py

+def retry(state, realtime):
+    state.set_status('')
+    if realtime:
+        # TODO: useless realtime. It's already in the if condition.


Let's address this by taking it out.

There are several places where we make this check. Are we going to call any of the methods when realtime is not true?

Yes, we definitely call parse_commands when realtime is not true.

jdm · 2018-03-13T20:11:43Z

tests/test_main.py

+    @patch('homu.main.PullReqState')
+    def test_review_approved_approver_me(self, MockPullReqState):
+        state = MockPullReqState()
+        self.assertFalse(review_approved(['r=me', 'approved'], 'r=me', state, True, 'user', 'user', 0, '', []))


We should have tests for non-realtime behavior as well.

jdm · 2018-03-13T20:12:28Z

tests/test_main.py

+        state = MockPullReqState()
+        state.title = 'WIP work in progress'
+        self.assertFalse(review_approved(['r+', 'approved'], 'r+', state, True, 'user', 'user', 0, '', []))
+        state.add_comment.assert_called_once_with(':clipboard: Looks like this PR is still in progress, ignoring approval')


We'll want some asserts that the rest of the state isn't updated when this happens.

jdm · 2018-03-13T20:13:05Z

tests/test_main.py

+        state = MockPullReqState()
+        state.title = 'TODO work in progress'
+        self.assertFalse(review_approved(['r+', 'approved'], 'r+', state, True, 'user', 'user', 0, '', []))
+        state.add_comment.assert_called_once_with(':clipboard: Looks like this PR is still in progress, ignoring approval')


This seems like it could be merged with the previous test to reduce duplication.

jdm · 2018-03-13T20:15:05Z

tests/test_main.py

+        self.assertFalse(state.try_)
+        state.set_status.assert_called_once_with('')
+        state.save.assert_called_once()
+        state.add_comment.assert_called_once_with(":bulb: This pull request was already approved, no need to approve it again.\n\n- This pull request is currently being tested. If there's no response from the continuous integration service, you may use `retry` to trigger a build again.")


It might be nice to have these strings in a common variable that can be used in the implementation and the test.

jdm · 2018-03-13T20:21:55Z

tests/test_main.py

+        state.repo_label = 'label'
+        state.status = 'pending'
+        states = {}
+        states[state.repo_label] = {'label': state}


It's not clear to me what this line is for. I see an iteration over states in review_approved, but no code checks a label property as far as I can see. The same question applies for all of the other tests that set it.

states[state.repo_label] should return a dict, because we call states[state.repo_label].values() in review_approved. What matters in this test is the state object. label is just an arbitrary name.

alexrs · 2018-03-13T21:08:57Z

Thanks for the thorough review! I'll address all these comments ASAP!

alexrs · 2018-03-15T11:13:08Z

Hey! Is there anything else I have to fix for this PR? I think we shouldn't add more code to it (apart from fixes to the current code). In a future PR, I'd like to remove the global configuration (global_cfg), extract the DB logic to its own class and wrap it to be able to mock it, which will also remove db_query_lock.
Once we have done that, we will be able to test parse_commands. What do you think about this approach?

jdm · 2018-03-15T16:55:37Z

This looks like a useful step forward. Thanks for working on it!
@bors-servo r+

bors-servo · 2018-03-15T16:55:38Z

📌 Commit cd5b1b5 has been approved by jdm

bors-servo · 2018-03-15T16:55:41Z

⌛ Testing commit cd5b1b5 with merge 2467053...

Refactor parse_commands to add tests. This PR address #143. This is a first step in making Homu testable. A small refactor has been done, extracting the logic in `parse_commands` to its own functions, to be able to mock the state and add tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/homu/149)

bors-servo · 2018-03-15T16:57:00Z

☀️ Test successful - status-travis
Approved by: jdm
Pushing 2467053 to master...

jdm suggested changes Mar 13, 2018

View reviewed changes

alexrs and others added 9 commits March 14, 2018 23:44

Refactor parse_commands to add tests.

7840629

remove functools import

54df015

Execute tests in Travis

15a5825

Fix tests

a654e0f

Limit github.py version to be lower than 1

a6daeb0

Move install command.

c341d1a

Install dependencies using pip

c1476c6

Add python 3.6

3fe9223

Replace assert_called_once by assert_called_once_with

cd5b1b5

bors-servo merged commit cd5b1b5 into servo:master Mar 15, 2018

pietroalbini mentioned this pull request Mar 19, 2018

Permissions checks broken on master #150

Closed

jdm mentioned this pull request Mar 27, 2018

Use up to date github3 package #157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor parse_commands to add tests. #149

Refactor parse_commands to add tests. #149

alexrs commented Mar 13, 2018 •

edited by larsbergstrom

Loading

jdm left a comment

jdm Mar 13, 2018

alexrs Mar 14, 2018

jdm Mar 14, 2018

alexrs Mar 14, 2018

jdm Mar 14, 2018

alexrs Mar 14, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

alexrs Mar 13, 2018

jdm Mar 14, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

jdm Mar 13, 2018

alexrs Mar 14, 2018

alexrs commented Mar 13, 2018

alexrs commented Mar 15, 2018

jdm commented Mar 15, 2018

bors-servo commented Mar 15, 2018

bors-servo commented Mar 15, 2018

bors-servo commented Mar 15, 2018

Refactor parse_commands to add tests. #149

Refactor parse_commands to add tests. #149

Conversation

alexrs commented Mar 13, 2018 • edited by larsbergstrom Loading

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrs commented Mar 13, 2018

alexrs commented Mar 15, 2018

jdm commented Mar 15, 2018

bors-servo commented Mar 15, 2018

bors-servo commented Mar 15, 2018

bors-servo commented Mar 15, 2018

alexrs commented Mar 13, 2018 •

edited by larsbergstrom

Loading