chmp / ipytest

Pytest in IPython notebooks.
MIT License
314 stars 17 forks source link

Selecting individual tests in a cell #52

Closed pmeier closed 3 years ago

pmeier commented 3 years ago

I'm looking for a way to use pytest's selection syntax together with ipytest. Consider the following cell:

%%run_pytest[clean]

class TestFoo:
    def test_bar(self):
        pass

    def test_baz(self):
        pass

class TestSpam:
    def test_ham(self):
        pass

Normally, I can for example pass $FILE::TestFoo::test_baz to select an individual test. Since I don't know the temporary file created by ipytest I can't do that here.

Would it be possible to simply prefix the temporary file in case ipytest encounters an argument starting with a double colon ::? Since there is no such pytest command, IMO it shouldn't break anything else.

pmeier commented 3 years ago

Ouch, just found #14. Will try that and report back.

pmeier commented 3 years ago

So #14 will not work for me. I want to showcase a pytest-plugin that interferes with the collection process. That means, I need to use the default syntax and cannot sidestep it with the regular expression matching (-k flag).

@chmp Would you accept a PR implementing this? It seems only this line needs to be adapted:

https://github.com/chmp/ipytest/blob/765d4be3944c8e2aeb1c5a737f54ea4a785adb3f/ipytest/_pytest_support.py#L72

chmp commented 3 years ago

TBH. I am a bit hesitant to add yet more config options, as ipytest is a bit overconfigurable already and your use case seems very specific. Would calling pytest manually work for you? I did a quick test and the following seems to work:

# file: Untitled.ipynb
# cell 1
class TestFoo:
    def test_bar(self):
        pass

    def test_baz(self):
        pass

class TestSpam:
    def test_ham(self):
        pass

# cell 2
import __main__
import pytest
from ipytest._pytest_support import ModuleCollectorPlugin

pytest.main(
    ["Untitled.ipynb::TestFoo::test_baz"],
    plugins=[ModuleCollectorPlugin(module=__main__, filename="Untitled.ipynb")]
)

If it helps, I would be happy to expose and document the plugin as part of the public API.

pmeier commented 3 years ago

TBH. I am a bit hesitant to add yet more config options, as ipytest is a bit overconfigurable already

Is that really an issue? I only discovered ipytest recently so you are the better judge, but IMO the combination of ipytest.autoconfig() while having the option to configure everything manually if needs be is a really good UX. Or is this about maintainability?

your use case seems very specific.

Indeed, it is. Still, such a feature would be turned off by default and shouldn't even interfere with other users code if it is on, I don't think that should be a reason to turn it down. Note that I'm not asking to add some special treatment for my use case, but rather being able to use a standard pytest feature together with ipytest.

Would calling pytest manually work for you?

Yes, your snippet works for my use-case. In particular for the use case of showcasing a pytest plugin it is very opaque for the reader. I would have to put a comment there, basically saying "Don't mind this mysterious code block. It lets us select individual tests.".

If it helps, I would be happy to expose and document the plugin as part of the public API.

If you decide to turn down my proposal, that would be great. Having to rely on private stuff is never a good idea.

pmeier commented 3 years ago

I've tinkered with your workaround a bit and found two more problems:

  1. It is dependent on the file name and at the same time the file name is not really accessible. That means I have to hard code it. Of course with this I always need an extra step in case I rename the file. Plus, it makes it less convenient to use as a snippet to paste into a new notebook,
  2. I lose the convenience of [clean]. When using the run_pytest[clean] cell magic, ipytest._util.clean_tests() is called before the tests in the current cell are registered. To replicate this functionality, I now need to call ipytest._util.clean_tests() before the newly defined tests and the snippet afterwards.
pmeier commented 3 years ago

My current work around looks like this:

import contextlib

import pytest
from ipytest._pytest_support import ModuleCollectorPlugin
from ipytest._util import clean_tests

import __main__

__file__ = "Untitled.ipynb"

@contextlib.contextmanager
def pytest_nb(*args: str, clean: bool = False) -> None:
    if args:
        args = [
            f"{__file__}{arg}" 
            if arg.startswith("::") 
            else arg 
            for arg in args
        ]
    else:
        args = [__file__]

    if clean:
        clean_tests(items=__main__.__dict__)

    yield

    pytest.main(
        args,
        plugins=[
            ModuleCollectorPlugin(
                module=__main__, 
                filename=__file__,
            )
        ]
    )

With that I can do this inside a cell:

with pytest_nb("--collect-only", "--quiet", "::TestFoo::test_baz"):

    class TestFoo:
        def test_bar(self):
            pass

        def test_baz(self):
            pass

    class TestSpam:
        def test_ham(self):
            pass
Untitled.py::TestFoo::test_baz

1 test collected in 0.00s
chmp commented 3 years ago

If you decide to turn down my proposal,

TBH. I'm not quite sure what your proposal is. If it's this part:

    if args:
        args = [
            f"{__file__}{arg}" 
            if arg.startswith("::") 
            else arg 
            for arg in args
        ]
    else:
        args = [__file__]

I have to admit, I'm not a fan. Due to two reasons:

  1. I typically use the args to add extra arguments, but I still want ipytest to add the current filename. E.g., I use %%run_pytest[clean] -x --pdb to debug test failures. Your change would break my (and I expect other people's) workflow. Anything you propose should not fundamentally change the interface without configuration by the user.
  2. The modification of arguments that start with "::" is every specific and introduces completely new semantics.

What I could imagine would work is:

  1. Adding a new config defopts that gives the default options that are always added to the CLI
  2. Introducing the new semantics that sth. like ${MODULE} in an argument is replaced with the current __file__ (or the tempfile name)
  3. Setting the default of to defopts=["${MODIULE}"]

Then your use case would be covered by

ipytest.autoconfig(defopts=[])

%%run_pytest[clean] ${MODULE}::class::method
%%run_pytest[clean] --collect-only --quiet ${MODULE}::TestFoo::test_baz
%%run_pytest[clean] ${MODULE}

I see two issues:

  1. Introducing a new config variable. Which I am not really a fan of.
  2. Introducing the "${MODULE}", but this change seems very contained.

As for the first point, potential alternatives would be:

  1. Reusing the addopts argument. But that would require users to add ${MODULE} manually, if they configure addopts
  2. Build some kind of parser that checks whether "test sources" (files or the like) are passed manually. Maybe it's as simple as checking whether there are any non-option arguments. However, I expect that this is much harder with plugins that can register their own command line option.

In any case I will expose the pytest plugin in the coming days and think about what could be a minimal change to the interface that would support your use case.

As for your code, you could always make it an IPython magic. If you have a look at the current impl, it's not super complicated. And you could put the code in an extra .py module to make the notebook less clutered.

pmeier commented 3 years ago

TBH. I'm not quite sure what your proposal is.

Fair enough, lets make this more concrete: if I get my will, I would add an option enable_relative_discovery: bool = False to the configuration. If this is set to True it would go through all args passed to the run_pytest cell magic and prefix everything that starts with :: with the valid_filename determined by

https://github.com/chmp/ipytest/blob/765d4be3944c8e2aeb1c5a737f54ea4a785adb3f/ipytest/_pytest_support.py#L100

  1. I typically use the args to add extra arguments, but I still want ipytest to add the current filename.

The snippet I posted was just what worked for me in my notebook. It was not meant for general use, but rather a proof of concept. If we go with what I proposed above, this would not be an issue.

The modification of arguments that start with "::" is every specific and introduces completely new semantics.

Only partially true. The double colon is standard pytest notation to separate modules, test cases, and test case functions. Since ipytest has the ability to automatically determine the file, it feels quite intuitive to me: "if I start my selection with double colons, ipytest should use whatever it determines as module."

chmp commented 3 years ago

If I understood you use case correctly, you also would like to prevent default discovery in the current notebook. For this to work, you also need to prevent the default addition of the current filename. With your proposal, you would end up with an equilvalent command line of:

# $MODULE: as the current filename
python -m pytest ${MODULE}::TestFoo::test_baz $MODULE

Which is not what you would like, I guess.

chmp commented 3 years ago

As for the argument mod: there could be plugins that interpret "::" differently and suddenly you break these plugins

chmp commented 3 years ago

I pushed an implementation to develop that should work for you. Configure:

import ipytest
ipytest.autoconfig(defopts=False)

and then use:

%%run_pytest[clean] {MODULE}::TestFoo::test_baz

class TestFoo:
    def test_bar(self):
        pass

    def test_baz(self):
        pass

class TestSpam:
    def test_ham(self):
        pass

Maybe you can give it a try. In the meantime I will think about potential alternatives.

pmeier commented 3 years ago

If I understood you use case correctly, you also would like to prevent default discovery in the current notebook.

I finally understand where you getting at. Thanks for the clarification. Yes, you are right, my proposal was too short-sighted.

there could be plugins that interpret "::" differently and suddenly you break these plugins

Also true.


Your solution seems like a good comprise. I'll give it a try and get back to you.

pmeier commented 3 years ago

@chmp Works like a charm. Thanks a lot for the support.

Would you be so kind to push a new release with that so I can access it through PyPI?

chmp commented 3 years ago

Sure will do. There are some other changes I would like to implement in the same release (mostly cleaning up the interface), but I should be able to push a pre-release the coming days.

chmp commented 3 years ago

I pushed a pre-release to pypi. You can install it with pip install ipytest==0.10.0b1.