Issue 3894 - Add tests for the update_issues hook

Issue 3894 - Add tests for the update_issues hook

Repository: adblockplus.org/sitescripts

[Vasily Kuznetsov, 2016-04-07 14:17:22]

Hi Felix,

See below my comments.

I think some of the issues that I saw are caused by some different setup when I
run the test. Is the right way to do it documented somewhere? Or maybe it would
be even better if we could just `hg clone ..../sitescripts` and then run all
tests with one command. So let me know if I'm doing it wrong.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
File sitescripts/hg/test/update_issues.py (right):

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:21: def _create_mock_repo(node, message):
It seems that `repo` is essentially a list of changes as far as the hook is
concerned. Could we just `return [mock_change]`?

This approach would also work well if we want to test the hook for multiple
changes, which I think might be a useful test case too.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:35: def
test_commits_with_invalid_message_format_ignored(self, mock_server_proxy):
Nit: this line is 80 characters long. PEP8 recommends 79 max.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:38: node = 0
I think the `node` variable is a bit redundant here and in other tests. You
essentially always create a mock repo with one change and then test the hook on
this change. So `_create_mock_repo` doesn't really need `node` and you could
just give `0` to the hook as a last parameter, I think it's clear enough.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:41: self.ui.warn.assert_called_once()
Does this work for you? When I run the test it fails on the second iteration of
the loop, which actually kind of makes sense: `self.ui` is only initialised once
per test and is not reset between the iterations.

Maybe we have different version of `mock` (mine is 2.0.0)?

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:55: def test_single_issue_referenced(self,
mock_server_proxy):
This also fails for me. Looks like the same problem as above.

[Felix Dahlke, 2016-04-07 15:42:00]

Thanks, new patch set is up.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
File sitescripts/hg/test/update_issues.py (right):

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:21: def _create_mock_repo(node, message):
On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> It seems that `repo` is essentially a list of changes as far as the hook is
> concerned. Could we just `return [mock_change]`?
> 
> This approach would also work well if we want to test the hook for multiple
> changes, which I think might be a useful test case too.

It's unfortunately not just a list, there are some other properties/methods on
it that the code uses. We could of course change _create_mock_repo to accept a
list of changes. I thought about that and then thought I'll just keep it simple,
until we actually need to test something with multiple changes. But no strong
opinion, if you prefer that (or have another idea).

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:35: def
test_commits_with_invalid_message_format_ignored(self, mock_server_proxy):
On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> Nit: this line is 80 characters long. PEP8 recommends 79 max.

Done.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:38: node = 0
On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> I think the `node` variable is a bit redundant here and in other tests. You
> essentially always create a mock repo with one change and then test the hook
on
> this change. So `_create_mock_repo` doesn't really need `node` and you could
> just give `0` to the hook as a last parameter, I think it's clear enough.

Done.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:41: self.ui.warn.assert_called_once()
On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> Does this work for you? When I run the test it fails on the second iteration
of
> the loop, which actually kind of makes sense: `self.ui` is only initialised
once
> per test and is not reset between the iterations.
> 
> Maybe we have different version of `mock` (mine is 2.0.0)?

Done.

I originally did reset the mocks. Then checked if they were needed and removed
them because they weren't in mock 1.0.1 for some reason. But in mock 2.0.0 it
fails indeed, fixed that.

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:55: def test_single_issue_referenced(self,
mock_server_proxy):
On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> This also fails for me. Looks like the same problem as above.

Done.

[Vasily Kuznetsov, 2016-04-07 21:16:49]

LGTM

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
File sitescripts/hg/test/update_issues.py (right):

https://codereview.adblockplus.org/29339548/diff/29339549/sitescripts/hg/test...
sitescripts/hg/test/update_issues.py:21: def _create_mock_repo(node, message):
On 2016/04/07 15:42:00, Felix Dahlke wrote:
> On 2016/04/07 14:17:22, Vasily Kuznetsov wrote:
> > It seems that `repo` is essentially a list of changes as far as the hook is
> > concerned. Could we just `return [mock_change]`?
> > 
> > This approach would also work well if we want to test the hook for multiple
> > changes, which I think might be a useful test case too.
> 
> It's unfortunately not just a list, there are some other properties/methods on
> it that the code uses. We could of course change _create_mock_repo to accept a
> list of changes. I thought about that and then thought I'll just keep it
simple,
> until we actually need to test something with multiple changes. But no strong
> opinion, if you prefer that (or have another idea).

Ok, yeah, I see, it does repo.url(). Then you'd have to subclass list to make it
work and your way is easier for just one change. Makes sense.

[kzar, 2016-04-13 12:56:29]

Sorry for the delay, LGTM