Re: [Tarantool-patches] [PATCH v3] Add exclude option as suite/test pattern

["Alexander Tikhonov" <avtikhon@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5410 bytes --]




>Среда, 23 октября 2019, 22:10 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
>
>We need to implement it in 'app' and 'unittest' tests too. See AppServer
>and UnittestServer classes (find_tests() method).
>
>See other notes below.
>
>WBR, Alexander Turenko.
>
>On Fri, Oct 18, 2019 at 10:13:25AM +0300, Alexander V. Tikhonov wrote:
>> Added exclude option to be able to exclude the tests
>> by patterns suite/test from testing, it means that
>> exclude option even excludes the suites given with
>> option suite.
>> Also added ability to set the list of exclude patterns
>> by the environment variable TEST_RUN_EXCLUDE_TESTS.
>> 
>> Close #54
>> ---
>> 
>> Github:  https://github.com/tarantool/test-run/tree/avtikhon/gh-54-exclude-option
>> Issue:  https://github.com/tarantool/test-run/issues/54
>> 
>>  lib/options.py          | 12 ++++++++++++
>>  lib/tarantool_server.py | 21 +++++++++++++++++++--
>>  2 files changed, 31 insertions(+), 2 deletions(-)
>> 
>> diff --git a/lib/options.py b/lib/options.py
>> index 94dbd17..4dad227 100644
>> --- a/lib/options.py
>> +++ b/lib/options.py
>> @@ -49,6 +49,18 @@ class Options:
>>                  tests starting with "show" in "box" suite. Default: run all
>>                  tests in all specified suites.""")
>> 
>> +        parser.add_argument(
>> +                "--exclude",
>> +                metavar = "exclude",
>
>Spaces around an equal sign in a call arguments are not recommended by
>PEP 8, consider `make lint`:
Corrected.
>
> | $ make lint
> | python2 -m flake8 *.py lib/*.py
> | lib/options.py:54:24: E251 unexpected spaces around keyword / parameter equals
> | lib/options.py:54:26: E251 unexpected spaces around keyword / parameter equals
> | lib/options.py:56:24: E251 unexpected spaces around keyword / parameter equals
> | lib/options.py:56:26: E251 unexpected spaces around keyword / parameter equals
> | make: *** [Makefile:6: lint] Error 1
>
>> +                nargs="*",
>
>So it will accepts several arguments?
>
>Consider the following command:
>
> | ./test-run.py foo --exclude bar baz
>
>How do you think, should it include or exclude 'baz' pattern?
>
>I tentative myself, to be honest, but I guess I would expect that 'baz'
>will be included. What do you think?
Changed the logic of the 'exclude' option which can have only single argument, but can be used multiply times.
>
>(Yep, I see, the same with --suite, but anyway.)
>
>> +                default = env_list('TEST_RUN_EXCLUDE_TESTS', ['']),
>> +                help="""Can be empty. List of excluding test names, to look for in
>> +                suites. Each name is used as a substring to look for in the
>> +                path to test file, e.g. "show" will run all tests that have
>> +                "show" in their name in all suites, "box/show" will only enable
>
>will run -> will exclude?
>enable -> disable? 
Fixed.
>
>
>> +                tests starting with "show" in "box" suite. Default: "" -
>> +                means no tests to exclude""")
>> +
>>          parser.add_argument(
>>                  "--suite",
>>                  dest='suites',
>> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
>> index 0340ada..d857c01 100644
>> --- a/lib/tarantool_server.py
>> +++ b/lib/tarantool_server.py
>> @@ -1102,13 +1102,30 @@ class TarantoolServer(Server):
>>              else:
>>                  tests.append(LuaTest(k, test_suite.args, test_suite.ini))
>> 
>> -        test_suite.tests = []
>> +        found_tests = []
>>          # don't sort, command line arguments must be run in
>>          # the specified order
>> +        # loop the list of test patterns need to run
>>          for name in test_suite.args.tests:
>> +            # loop the list of found tests at the sources
>>              for test in tests:
>> +                # check if the test matches the needed pattern
>>                  if test.name.find(name) != -1:
>> -                    test_suite.tests.append(test)
>> +                    found_tests.append(test)
>> +
>> +        test_suite.tests = []
>> +        exclude_tests = test_suite.args.exclude
>> +        # check if any of the exclude patterns set
>> +        if exclude_tests != ['']:
>
>I would choose such default value that the check would be `if
>exclude_tests` (because more simple is more pythonic). An empty list
>would be good choice I think.
Changed.
>
>Also we can decrease a indentation level using for/else construction.
>See  http://book.pythontips.com/en/latest/for_-_else.html
Changed.
>
>> +            # loop the list of test patterns need to exclude
>> +            for name in exclude_tests:
>> +                # loop the list of found tests need to run
>> +                for test in found_tests:
>> +                    # check if the test not matches the pattern
>> +                    if test.name.find(name) == -1:
>> +                        test_suite.tests.append(test)
>
>I think we should exclude a test if it matches **any** of exclude
>patterns. Also I guess the current implementation will add a test
>several times if several exclude patterns are provided and there are
>several of them that does not match the test.
Corrected.
>
>> +        else:
>> +            test_suite.tests = found_tests
>> 
>>      def get_param(self, param=None):
>>          if param is not None:
>> -- 
>> 2.17.1
>> 


-- 
Alexander Tikhonov

[-- Attachment #2: Type: text/html, Size: 7957 bytes --]