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

[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`:

 | $ 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?

(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?

> +                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.

Also we can decrease a indentation level using for/else construction.
See http://book.pythontips.com/en/latest/for_-_else.html

> +            # 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.

> +        else:
> +            test_suite.tests = found_tests
>  
>      def get_param(self, param=None):
>          if param is not None:
> -- 
> 2.17.1
>