Running multiple test cases across different files (crash via -k (twice))

[otrejoso]

Quick question, I am trying to run multiple test cases across different files using -k, but I am getting the following error:

(venv3.7) C:\depot\bitbucket\test\example>pytest -v test_example_suite_2.py test_example_suite_2.py -k "test_1[1] and test_3[5]"
=========================================================================================================== test session starts ============================================================================================================
platform win32 -- Python 3.7.2, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- c:\depot\bitbucket\test\venv3.7\scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\depot\bitbucket\test\example
collected 6 items

========================================================================================================== no tests ran in 0.02s ===========================================================================================================
Traceback (most recent call last):
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 197, in wrap_session
    session.exitstatus = doit(config, session) or 0
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 246, in _main
    config.hook.pytest_collection(session=session)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 208, in _multicall
    return outcome.get_result()
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 256, in pytest_collection
    return session.perform_collect()
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 461, in perform_collect
    session=self, config=self.config, items=items
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 208, in _multicall
    return outcome.get_result()
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\mark\__init__.py", line 142, in pytest_collection_modifyitems
    deselect_by_keyword(items, config)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\mark\__init__.py", line 111, in deselect_by_keyword
    if keywordexpr and not matchkeyword(colitem, keywordexpr):
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\mark\legacy.py", line 100, in matchkeyword
    return eval(keywordexpr, {}, mapping)
  File "<string>", line 1, in <module>
TypeError: 'bool' object is not subscriptable

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Python37\Lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Python37\Lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\depot\bitbucket\test\venv3.7\Scripts\pytest.exe\__main__.py", line 7, in <module>
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\config\__init__.py", line 93, in main
    config=config
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 208, in _multicall
    return outcome.get_result()
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 240, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\main.py", line 219, in wrap_session
    config.notify_exception(excinfo, config.option)
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\config\__init__.py", line 822, in notify_exception
    funcargs=True, showlocals=getattr(option, "showlocals", False), style=style
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\_code\code.py", line 619, in getrepr
    self._getreprcrash(),
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\_code\code.py", line 570, in _getreprcrash
    entry = self.traceback.getcrashentry()
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\_code\code.py", line 383, in getcrashentry
    if not entry.ishidden():
  File "c:\depot\bitbucket\test\venv3.7\lib\site-packages\_pytest\_code\code.py", line 264, in ishidden
    tbh = f.f_locals.get(
AttributeError: 'KeywordMapping' object has no attribute 'get'

Are we doing this right? or is this a bug?

This is our enviroment

(venv3.7) C:\depot\bitbucket\test\example>pip list
Package            Version
------------------ -------
atomicwrites       1.3.0
attrs              19.3.0
colorama           0.4.3
importlib-metadata 1.5.0
more-itertools     8.2.0
packaging          20.1
pip                20.0.2
pluggy             0.13.1
py                 1.8.1
pyparsing          2.4.6
pytest             5.3.5
setuptools         45.2.0
six                1.14.0
wcwidth            0.1.8
wheel              0.34.2
zipp               3.0.0

[Zac-HD]

I'm not entirely sure what you're trying to do (showing the contents of the files would help!) or what should happen, but if we're showing stack traces that's a pytest bug too.

[RonnyPfannschmidt]

this is a known issue with keyword expressions - the are parsed as python expressions,

its impossible to use [....] in keyword expressions as of now, as the value of the name lookup is a plain boolean and no extension for interesting apis has been done7was planned

[otrejoso]

I see. I case you are interested about a repro, these are my files
test_example_suite_2.py

import pytest


class TestExample2:
    @pytest.mark.parametrize('foo', [1, 2])
    def test_1(self, foo):
        assert True

    def test_2(self):
        assert True

test_example_suite_3.py

import pytest


class TestExample3:
    @pytest.mark.parametrize('foo', [4, 5])
    def test_3(self, foo):
        assert True

    def test_4(self):
        assert True

[blueyed]

(it should certainly not crash like that at least (twice))

[gdhameeja]

I would like to work on this. Although this is my first time contributing to pytest, any guidance would be of great help. :)

[RonnyPfannschmidt]

the key issue, that we run the mark expression unchecked against a predefined name-space that will only ever contain "bools"

so operators and attribute access cant work, only name lookup and basic operators

a solution would be to do a basic check on the string given in to detect unsupported operations in some manner

[The-Compiler]

I'd say a first step would be to have better error handling/reporting for such errors in a -k argument. Maybe you'd like to start with that, @gdhameeja?

After that, I wonder if it's really worth continuing to work on the solution we have currently. At this point, it seems like it'd almost be easier to either hand-roll our own parser for those expressions or see what libraries there are we could use, rather than pretending something is Python code just so we can use and, or and not.

[RonnyPfannschmidt]

hmmm, we could go the easy way and simply disallow subscriptions and attribute access,

[nicoddemus]

Hi @gdhameeja,

Thanks for working on #6854!

Unfortunately just checking for . in the mapping is not enough, because -k 1.3 is something valid according to the documentation:

However, if the "-k" argument is a simple string, no such restrictions apply. Also "-k 'not STRING'" has no restrictions. You can also specify numbers like "-k 1.3" to match tests which are parametrized with the float "1.3".

(That's why also tests have failed).

I think @The-Compiler has a point about us change to trying to parse the expression instead of calling eval on it. Does only and, not and or make sense currently?

I'd say a first step would be to have better error handling/reporting for such errors in a -k argument. Maybe you'd like to start with that, @gdhameeja?

I second that, that should be simpler and a great improvement already. 👍

I think just catching Exception instead of SyntaxError in here:

pytest/src/_pytest/mark/legacy.py

Lines 107 to 110 in acec0b6

	try:
	return eval(keywordexpr, {}, mapping)
	except SyntaxError:
	raise UsageError("Wrong expression passed to '-k': {}".format(keywordexpr))

Would be enough for a better message?

[bluetech]

I think @The-Compiler has a point about us change to trying to parse the expression instead of calling eval on it. Does only and, not and or make sense currently?

I also agree we should parse manually and not use eval. Will probably simplify the code, too (get rid of KeywordMapping etc.). I can look into it unless someone else started to (@nicoddemus?).

[RonnyPfannschmidt]

Unless anyone has a plan to do this without accidentally creating a breaking change, i would strongly recommend to just do basic checking /error handling

Both mark and keyword expressions are a problem

[gdhameeja]

@bluetech I'd like to work on removing eval and parsing manually. But since I'm new to this idea, (and pytest) itself, I'd need some guidance. Can I please take this?

[bluetech]

Unless anyone has a plan to do this without accidentally creating a breaking change, i would strongly recommend to just do basic checking /error handling

I think compatibility should be preserved. Of course if it accidentally broken, then it should be treated as a bug an fixed. Although it depends on the details.

[bluetech]

@gdhameeja

@bluetech I'd like to work on removing eval and parsing manually. But since I'm new to this idea, (and pytest) itself, I'd need some guidance. Can I please take this?

Great! Here is how I would go about it:

---

First I would read the current code to understand how it works.

You can start with the deselect_by_keyword() function in src/_pytest/mark/__init__.py. This function takes -k option ("keyword") and the list of items, and modifies the list of items according to the keyword.

This function calls into the matchkeyword() function in src/_pytest/mark/legacy.py which takes an item and the keyword expression and returns whether it matches or not. This is probably the main function you'd want to rewrite.

What matchkeyword does is it evaluates the keyword expression as a Python expression, using Python's eval function. Make sure to read its documentation: https://docs.python.org/3/library/functions.html#eval and especially understand its two arguments locals and globals.

matchkeyword uses the globals argument to create a context for the expression in which certain names (like the item's name) evaluate to True, and others to False. This is done by the KeywordMapping class whose __getitem__ performs the check.

---

Now, the idea is the get rid of the eval, which is ill-specified and dangerous. You can do it however you think is best, but I would go about it like this:

First I'd specify a formal grammar for the expression, which should determine exactly which expressions are well-formed (have proper syntax) and which are not. You can see for example Python's own grammar here, but note this one is much simpler, probably only a few lines to specify a Boolean expression with identifiers. You should be able to do it after studying the existing code.

After you specify the syntax, you need to specify the semantics, i.e. how to determine whether a given well-formed expression evaluates to True or False. You should also be to do this after studying the existing code.

Then you can do the fun part which is implementing it. But before doing that, would probably wise to post a comment here with what you discovered and your plan.

---

I hope I didn't make it sound too complicated. In the end, it shouldn't be.

[RonnyPfannschmidt]

I would suggest to just parse to a python ast and blacklisting certain nodes

[bluetech]

Using Python's ast still has some disadvantages, for example this note from the docs:

If you are using expressions such as "X and Y" then both X and Y need to be simple non-keyword names. For example, "pass" or "from" will result in SyntaxErrors because "-k" evaluates the expression using Python’s eval function.

However, if the "-k" argument is a simple string, no such restrictions apply. Also "-k 'not STRING'" has no restrictions. You can also specify numbers like "-k 1.3" to match tests which are parametrized with the float "1.3".

Such notes should not be necessary I think...

[RonnyPfannschmidt]

Going from a blacklist to ast construction is going to be simpler than inventing a new grammar

Plus testing can be generalised

[The-Compiler]

I'm still -1 about pretending something is Python when it isn't, and then adding a pile of hacks on top of it to somehow shoehorn it into working (for some cases).

Accidental breaking changes happen, that shouldn't prevent us from making things simpler. Certainly wouldn't be the first time we accidentally broke something in the process, but if that happens, we can always improve and cut a new release.

I still think a grammar to support anything which currently is valid (and does something sensible) would be trivial. Maybe I'm wrong, but it'd nice to see some evidence that anything other than:

• expr
• expr and expr
• expr or expr
• not expr
• combinations thereof
• parentheses

is really supported and used at the moment. It certainly isn't documented as being supported.