Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(472)

Issue 29588962: Issue 5934 - CMS testing automation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by Vasily Kuznetsov
Modified:
2 years, 3 months ago
Visibility:
Public.

Description

Issue 5934 - CMS testing automation Repository: https://hg.adblockplus.org/codingtools/ Base revision: 78f5f8db43d1

Patch Set 1 #

Patch Set 2 : Change the usage a bit, add some useful options, add a couple of tests #

Total comments: 13

Patch Set 3 : Improve documentation and structure following Tristan's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+397 lines, -0 lines) Patch
A cms-dev/cms_cmp.py View 1 2 1 chunk +250 lines, -0 lines 0 comments Download
A cms-dev/tests/test_cms_cmp.py View 1 2 1 chunk +127 lines, -0 lines 0 comments Download
A cms-dev/tox.ini View 1 2 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 5
Vasily Kuznetsov
Hi all, I think we might need a README for this, but I thought I'd ...
2 years, 5 months ago (2017-10-25 16:50:03 UTC) #1
Vasily Kuznetsov
After using this script a bit more I changed the invocation a bit and then ...
2 years, 5 months ago (2017-10-27 17:39:53 UTC) #2
tlucas
Hey Vasily! Only some philosophical comments and one weird problem with tox/flake8, please see below ...
2 years, 4 months ago (2017-11-24 10:57:46 UTC) #3
Vasily Kuznetsov
Hi Tristan, Thank you for the feedback. I've addressed most of you comments (with the ...
2 years, 4 months ago (2017-11-28 16:53:38 UTC) #4
tlucas
2 years, 3 months ago (2017-12-14 10:30:43 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5