Issue 5934 - CMS testing automation

Issue 5934 - CMS testing automation

Repository: https://hg.adblockplus.org/codingtools/
Base revision: 78f5f8db43d1

[Vasily Kuznetsov, 2017-10-25 16:50:03]

Hi all,

I think we might need a README for this, but I thought I'd upload the code
already and see what you guys think.

Cheers,
Vasily

[Vasily Kuznetsov, 2017-10-27 17:39:53]

After using this script a bit more I changed the invocation a bit and then
decided to add some tests.

[tlucas, 2017-11-24 10:57:46]

Hey Vasily!

Only some philosophical comments and one weird problem with tox/flake8, please
see below :)

Best,
Tristan

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py
File cms-dev/cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py#...
cms-dev/cms_cmp.py:172: def configure():
Since configure()'s only purpose is to specify parameters for Tester, what do
you think about making it a Tester method, called within __init__()?

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
File cms-dev/tests/test_cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
cms-dev/tests/test_cms_cmp.py:30: def hg(*args, **kw):
This is a copy of cms_cmp.hg(), without the call of run_cmd() - is there any
specific reason, why you are not simply importing it?

Imho the tests would also benefit from the beautified error message you print in
cmp_cms.run_cmd().

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
cms-dev/tests/test_cms_cmp.py:121: # This will fail because surrent version is
'other'.
typo "current"

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini
File cms-dev/tox.ini (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini#new...
cms-dev/tox.ini:6: ignore = D1
Do we want to keep ignoring all D1 errors in new code? While i agree that it
would be overkill for tests, the actual API would IMHO benefit from it.

(Besides, the only 2 D1 warnings in cmp_cms.py would be:

cms_cmp.py:116:1: D107 Missing docstring in __init__
cms_cmp.py:159:1: D102 Missing docstring in public method
)

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini#new...
cms-dev/tox.ini:14: [testenv:flake8]
Something is screwy in here.

running "$ tox" yields no warnings at all, activating the flake8 environment
generated by tox and running "$ flake8 cms-cmp.py tests" yields the following:

./cms_cmp.py:83:1: D202 No blank lines allowed after function docstring
./cms_cmp.py:203:1: D401 First line should be in imperative mood; try rephrasing

[Vasily Kuznetsov, 2017-11-28 16:53:38]

Hi Tristan,

Thank you for the feedback. I've addressed most of you comments (with the
exception of sharing the code for `hg` invocation).

Cheers,
Vasily

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py
File cms-dev/cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py#...
cms-dev/cms_cmp.py:172: def configure():
On 2017/11/24 10:57:46, tlucas wrote:
> Since configure()'s only purpose is to specify parameters for Tester, what do
> you think about making it a Tester method, called within __init__()?

I would like to keep `Tester` independent from particulars of option parsing, so
my preference would be to keep the option parsing out of it. You could say that
it's not really independent since it expects an `argparse` namespace in the
constructor, and I would agree with this. Still, I would prefer to go towards
greater separation of option parsing from CMS running and comparisons, perhaps
via passing keyword arguments to `Tester.__init__` instead of an option
namespace.

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
File cms-dev/tests/test_cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
cms-dev/tests/test_cms_cmp.py:30: def hg(*args, **kw):
On 2017/11/24 10:57:46, tlucas wrote:
> This is a copy of cms_cmp.hg(), without the call of run_cmd() - is there any
> specific reason, why you are not simply importing it?
> 
> Imho the tests would also benefit from the beautified error message you print
in
> cmp_cms.run_cmd().

I had in mind three arguments for implementing a separate `hg()` function here:
  1. We don't want to use the code under test for running the tests.
  2. `run_cmd()` throws `SystemExit` exception, which normally leads to process
exit.
  3. We need to set `PYTHONPATH` in order to import `cms_cmp.py` and that's kind
of ugly.

However, (1) is not a very strong argument and (2) is based on a faulty
assumption (actually, pytest catches `SystemExit` and reports it  as a test
failure, so the behavior of tests, in which `hg` fails, would not change). (3)
is valid, but should instead be solved by making this a python package (which I
thought was a bit of an overkill).

All in all, I'm leaning towards following your advice as soon as this is made
into a Python package, but perhaps not right now. What do you think?

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
cms-dev/tests/test_cms_cmp.py:121: # This will fail because surrent version is
'other'.
On 2017/11/24 10:57:46, tlucas wrote:
> typo "current"

Done.

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini
File cms-dev/tox.ini (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini#new...
cms-dev/tox.ini:6: ignore = D1
On 2017/11/24 10:57:46, tlucas wrote:
> Do we want to keep ignoring all D1 errors in new code? While i agree that it
> would be overkill for tests, the actual API would IMHO benefit from it.
> 
> (Besides, the only 2 D1 warnings in cmp_cms.py would be:
> 
> cms_cmp.py:116:1: D107 Missing docstring in __init__
> cms_cmp.py:159:1: D102 Missing docstring in public method
> )

This is not really a public API, but I agree with you and I would rather add
missing docstrings and remove the ignore.

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini#new...
cms-dev/tox.ini:6: ignore = D1
On 2017/11/24 10:57:46, tlucas wrote:
> Do we want to keep ignoring all D1 errors in new code? While i agree that it
> would be overkill for tests, the actual API would IMHO benefit from it.
> 
> (Besides, the only 2 D1 warnings in cmp_cms.py would be:
> 
> cms_cmp.py:116:1: D107 Missing docstring in __init__
> cms_cmp.py:159:1: D102 Missing docstring in public method
> )

Done.

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tox.ini#new...
cms-dev/tox.ini:14: [testenv:flake8]
On 2017/11/24 10:57:46, tlucas wrote:
> Something is screwy in here.
> 
> running "$ tox" yields no warnings at all, activating the flake8 environment
> generated by tox and running "$ flake8 cms-cmp.py tests" yields the following:
> 
> ./cms_cmp.py:83:1: D202 No blank lines allowed after function docstring
> ./cms_cmp.py:203:1: D401 First line should be in imperative mood; try
rephrasing

Done.

[tlucas, 2017-12-14 10:30:43]

Hey Vasily,

LGTM (so far ;) )

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py
File cms-dev/cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/cms_cmp.py#...
cms-dev/cms_cmp.py:172: def configure():
On 2017/11/28 16:53:37, Vasily Kuznetsov wrote:
> On 2017/11/24 10:57:46, tlucas wrote:
> > Since configure()'s only purpose is to specify parameters for Tester, what
do
> > you think about making it a Tester method, called within __init__()?
> 
> I would like to keep `Tester` independent from particulars of option parsing,
so
> my preference would be to keep the option parsing out of it. You could say
that
> it's not really independent since it expects an `argparse` namespace in the
> constructor, and I would agree with this. Still, I would prefer to go towards
> greater separation of option parsing from CMS running and comparisons, perhaps
> via passing keyword arguments to `Tester.__init__` instead of an option
> namespace.

ACK, your argumentation seems valid :)

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
File cms-dev/tests/test_cms_cmp.py (right):

https://codereview.adblockplus.org/29588962/diff/29590616/cms-dev/tests/test_...
cms-dev/tests/test_cms_cmp.py:30: def hg(*args, **kw):
On 2017/11/28 16:53:37, Vasily Kuznetsov wrote:
> On 2017/11/24 10:57:46, tlucas wrote:
> > This is a copy of cms_cmp.hg(), without the call of run_cmd() - is there any
> > specific reason, why you are not simply importing it?
> > 
> > Imho the tests would also benefit from the beautified error message you
print
> in
> > cmp_cms.run_cmd().
> 
> I had in mind three arguments for implementing a separate `hg()` function
here:
>   1. We don't want to use the code under test for running the tests.
>   2. `run_cmd()` throws `SystemExit` exception, which normally leads to
process
> exit.
>   3. We need to set `PYTHONPATH` in order to import `cms_cmp.py` and that's
kind
> of ugly.
> 
> However, (1) is not a very strong argument and (2) is based on a faulty
> assumption (actually, pytest catches `SystemExit` and reports it  as a test
> failure, so the behavior of tests, in which `hg` fails, would not change). (3)
> is valid, but should instead be solved by making this a python package (which
I
> thought was a bit of an overkill).
> 
> All in all, I'm leaning towards following your advice as soon as this is made
> into a Python package, but perhaps not right now. What do you think?

I agree, right now this is not a very urgent topic and yes, it would be nice to
someday have this as a python package.