Allow contibuting additional global variables for skipif/xfail

[s0undt3ch]

• Include documentation when adding new features.
• Include new tests or update existing tests when applicable.
• Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
• Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

[s0undt3ch]

Please advise on the hook name, what additional testing might be required, or anything else regarding the PR.

[RonnyPfannschmidt]

Something like pytest_markeval_namespace, it's tricky to name it well

[RonnyPfannschmidt]

Also thank you for digging into this,
That looks good and takes something off my minds backlog.

Now the truely hard part will be the correct name of the hook 😅

[bluetech]

I personally consider string conditions to be a legacy feature, kept for compatibility, and better replaced by regular boolean conditions. Can you describe your use case for this hook?

[RonnyPfannschmidt]

@bluetech i would like this to land even if strings eventually go "away", because they can be arguments to a lambda function

[bluetech]

Is there an example for a situation where this would be useful?

[RonnyPfannschmidt]

i have a direct use-case
that boils down to the following pseudo code

import attr

@attr.s
class AppUnderTestPlugin:
    _app = attr.s()

   @classmethod
   def from_config(cls, pytestconfig):
      ...
      return cls(...)
   

   def pytest_markeval_namespace(self):
     return {'application': self.app}

   @pytest.fixture
  def app(self):
    return self._app

def pytest_configure(config):
     config.pluginmanager.register(AppUnderTestPlugin.from_config(config))

test_fun.py

import pytest

@pytest.mark.xfail("application.use_beta", reason="only in stable"
def test_fun(app):
   ...

[s0undt3ch]

Our project, salt, is a multiple platform project, and, as such, we have tests targeted to specific platforms, and in some cases, to specific versions of said platforms. It constructs a dictionary which we call grains that includes several details about the platform, consider it distro on some serious steroids abuse.

Constructing this grains dictionary is an expensive runtime operation, and for the most part, these grains are static, so, constructing them on every module that needs to use them is unpractical. We do have the option to construct these grains in a global module variable and import it so that we don't have to use "string conditions", but that means a global mutable dictionary in a module scope and requiring it's import on every test module than needs to use them.

"String conditions" make this whole code a lot simpler, and it would be a pity to see if go away.

Example code:

@pytest.mark.skipif(
    "grains['os_family'] == 'RedHat' and grains.get('osmajorrelease') == 6",
    reason='RedHat 6 Specific Test'
)
def test_foo():
    assert False

[s0undt3ch]

Hmm, why isn't this line getting hit by tests? Do hooks ignore empty results?

[nicoddemus]

i have a direct use-case

I agree this is useful, because while this will be used only for string conditions (which I agree we might want to get away in the future, but not sure), the actual feature is "delayed" evaluation. As @RonnyPfannschmidt mentions, we can evolve this in the future to make the condition also be a callable which supports arbitrary keyword arguments. String conditions and callable conditions would then be evaluated at test setup time, and not during collection.

Adapting @RonnyPfannschmidt's example:

import pytest

@pytest.mark.xfail(lambda **kw: kw["application"].use_beta, reason="only in stable")
def test_fun(app):
   ...

Or @s0undt3ch's:

@pytest.mark.skipif(
    lambda **kw: kw["grains"]['os_family'] == 'RedHat' and kw["grains"].get('osmajorrelease') == 6,
    reason='RedHat 6 Specific Test'
)
def test_foo():
    assert False

I've also used string evaluation in modern pytest for the same reason.

[RonnyPfannschmidt]

The lamdas could take the argument names directly, no need for kw

[nicoddemus]

Suggested change

	def pytest_markeval_namespace() -> Dict[str, Any]:
	def pytest_mark_eval_namespace() -> Dict[str, Any]:

What do you think @RonnyPfannschmidt ?

Also I think we can add Config here as parameter:

Suggested change

	def pytest_markeval_namespace() -> Dict[str, Any]:
	def pytest_markeval_namespace(config: Config) -> Dict[str, Any]:

[nicoddemus]

The lamdas could take the argument names directly, no need for kw

If we inspect the function to only pass the arguments it declares (like we do with hooks), you are right of course. 👍

[s0undt3ch]

Ok.
After some crafty debug print statements,

diff --git a/src/_pytest/skipping.py b/src/_pytest/skipping.py
index 92916eb8b..dce3f90d8 100644
--- a/src/_pytest/skipping.py
+++ b/src/_pytest/skipping.py
@@ -103,6 +103,7 @@ def evaluate_condition(item: Item, mark: Mark, condition: object) -> Tuple[bool,
             "config": item.config,
         }
         for dictionary in item.ihook.pytest_markeval_namespace(config=item.config):
+            print(1, dictionary)
             if not isinstance(dictionary, Mapping):
                 raise ValueError(
                     "pytest_markeval_namespace() needs to return a dict, got {!r}".format(
@@ -110,6 +111,7 @@ def evaluate_condition(item: Item, mark: Mark, condition: object) -> Tuple[bool,
                     )
                 )
             globals_.update(dictionary)
+        print(2, globals_)
         if hasattr(item, "obj"):
             globals_.update(item.obj.__globals__)  # type: ignore[attr-defined]
         try:
diff --git a/testing/test_skipping.py b/testing/test_skipping.py
index 21a1bed66..c542e4703 100644
--- a/testing/test_skipping.py
+++ b/testing/test_skipping.py
@@ -190,6 +190,7 @@ class TestEvaluation:
             import pytest
 
             def pytest_markeval_namespace():
+                print("root")
                 return {"arg": "root"}
             """
             )
@@ -213,6 +214,7 @@ class TestEvaluation:
             import pytest
 
             def pytest_markeval_namespace():
+                print("foo")
                 return {"arg": "foo"}
             """
             )
@@ -236,6 +238,7 @@ class TestEvaluation:
             import pytest
 
             def pytest_markeval_namespace():
+                print("bar")
                 return {"arg": "bar"}
             """
             )

hooks are getting called nested first, top level last.

testing/test_skipping.py::TestEvaluation::test_skipif_markeval_namespace_multiple =============================================================================================== test session starts ==============
==================================================================================                                                                                                                                  
platform linux -- Python 3.7.5, pytest-6.0.1.dev154+gbf3dcce91.d20200830, py-1.9.0, pluggy-0.13.1 -- /home/vampas/projects/SaltStack/pytest/pytest/features/skip-context-hook/.tox/py37/bin/python                  
cachedir: .pytest_cache                              
rootdir: /tmp/pytest-of-vampas/pytest-27/test_skipif_markeval_namespace_multiple0
collected 3 items                                                                                                                                                                                                   
                                                     
root/test_root.py::test_root root                                                                         
1 {'arg': 'root'}                                    
2 {'os': <module 'os' from '/home/vampas/.dotfiles/.ext/pyenv/versions/3.7.5/lib/python3.7/os.py'>, 'sys': <module 'sys' (built-in)>, 'platform': <module 'platform' from '/home/vampas/.dotfiles/.ext/pyenv/versions/3.7.5/lib/python3.7/platform.py'>, 'config': <_pytest.config.Config object at 0x7fdf433ca850>, 'arg': 'root'}                                                                                                     
SKIPPED                                                                                                   
root/bar/test_bar.py::test_bar bar                                                                        
root                                        
1 {'arg': 'bar'}                        
1 {'arg': 'root'}                         
2 {'os': <module 'os' from '/home/vampas/.dotfiles/.ext/pyenv/versions/3.7.5/lib/python3.7/os.py'>, 'sys': <module 'sys' (built-in)>, 'platform': <module 'platform' from '/home/vampas/.dotfiles/.ext/pyenv/version
s/3.7.5/lib/python3.7/platform.py'>, 'config': <_pytest.config.Config object at 0x7fdf433ca850>, 'arg': 'root'}
FAILED                                                                                                                                                                                                              
root/foo/test_foo.py::test_foo foo                  
root                                                                                                      
1 {'arg': 'foo'}                                                                                                                                                                                                    
1 {'arg': 'root'}                                                                                                                                                                                                   
2 {'os': <module 'os' from '/home/vampas/.dotfiles/.ext/pyenv/versions/3.7.5/lib/python3.7/os.py'>, 'sys': <module 'sys' (built-in)>, 'platform': <module 'platform' from '/home/vampas/.dotfiles/.ext/pyenv/version
s/3.7.5/lib/python3.7/platform.py'>, 'config': <_pytest.config.Config object at 0x7fdf433ca850>, 'arg': 'root'}                                                                                                     
FAILED

Is this how hooks are supposed to be called? I was expecting top level first, nested last.
What't the approach to take here?

[bluetech]

Opened issue #7712.

[nicoddemus]

This can now be merged given we've released 6.1 this week. 🎉

@s0undt3ch do yout want to rebase and do the honors of squash/merging this? 😁

Also if anybody wants to review this now would be the time to speak up. 👍

[s0undt3ch]

Sure!
But, we're not waiting for @bluetech's fixture approach?

[nicoddemus]

But, we're not waiting for @bluetech's fixture approach?

Ahh sorry, yeah, I was reviewing 6.2's milestones and saw this, and forgot about the details. 🤦

[s0undt3ch]

Anyway, squashed and rebased.

[nicoddemus]

Thanks!