chmp / ipytest

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

Add 'how to run a specific test' to the docs #71

Closed axil closed 2 years ago

axil commented 2 years ago

Supposedly this runs a specific test:

import ipytest
ipytest.autoconfig()

def test1():
    assert 2*2 == 5

ipytest.config(defopts=False)
ipytest.run('{MODULE}::test1')

(btw, is there any way to pass defopts to run directly?)

and this runs a subset of tests with a certain substring in the name:

import ipytest
ipytest.autoconfig()

def test_this1():
    assert 2*2 == 11

def test_this2():
    assert 2*2 == 12

def test_that1():
    assert 2*2 == 21

def test_that2():
    assert 2*2 == 22    

ipytest.run('-k', 'this')

It was not obvious to figure it out :)

I would suggest adding it to the docs.

chmp commented 2 years ago

Hey @axil,

Thanks for these suggestions. You're right the pytest command line arguments are definitely not obvious. Maybe something for a "tips and tricks" section with pointer into the pytest docs (my favs are "-x --pdb" or "--ff").

Re giving the options directly: at the moment it is not possible, but I like it. Maybe having some way to specify them also via the magics would be cool. This way there is feature parity between the two ways of using ipytest.

chmp commented 2 years ago

I implemented the option to override defopts (and various other options) in ipytest.run and %%ipytest. I also expanded the example notebook with some pointers on helpful pytest options (following your suggestions).

To test your use case install the pre-relase 0.13.0a1:

pip install ipytest==0.13.0a1

and then run either:

def test1():
    assert 2*2 == 5

ipytest.run('{MODULE}::test1', defopts=False)

or

%%ipytest {MODULE}::test1
# ipytest: defopts=False

def test1():
    assert 2*2 == 5

Would be happy to hear, whether this works for you. I will test the pre-release myself for a bit and release it later this month, if everything works as intended.

axil commented 2 years ago

Wow, that was fast!

I can't say that ipytest.run('{MODULE}::test1', defopts=False) is something especially intuitive or user-friendly, but if it will be documented, maybe it is ok. At least this is something I had in my mind when I was posting a feature request :)

Alternatively, a keyword argument like

ipytest.run(tests=['test1', 'test5'])

would be more usable.

Thanks anyway!

chmp commented 2 years ago

Haha. I can't say that, as well.

TBH. I am hesitant to add too many additional interfaces on top of pytest, but maybe there is a middle ground that still solves this issue: How about detecting if there are non-option CLI arguments and then acting as if defopts=False were set. This should solve your use case and not interfere with the most common use case to add additional option arguments to pytest (-x, --pdb, -vv, ...).

With this change you could simply run ipytest.run('{MODULE}::test1').

axil commented 2 years ago

My usual testing workflow is: run all tests. If some of them fails, debug only those which failed. It doesn't make sense to run all tests if you only debug some of them. pytest is aimed at large projects with deep hierarchy of folders. Jupyter notebooks almost never have a complicated directory structure, so having to type module name, double colon, etc looks superfluous for it (in my opinion). Also the amount of keystrokes for running exactly two or three tests starts to be horrifying :) Ymmv.

chmp commented 2 years ago

Interesting. I split my tests across multiple cells with %%ipytest on top. Since each cells contains at most 5 - 10 tests, simply rerunning everything is not an issue. So far I never bothered to select a subset of tests.

I still don't like adding a separate tests argument to run as it is an abstraction on top of pytest (and also because it cannot be used with the magic and I aim for feature parity between the two). Maybe there is another way of designing this feature, that is not as intrusive, but still requires less typing. I will ponder it for a bit.

In any case, I feel this discussion already increased user friendliniess a lot. Thanks for that :)

chmp commented 2 years ago

Hm. My initial implementation of the defopts="auto" idea, is a bit too simplistic. It fails with command lines like -k test1. Back to the drawing board.

Edit: made sure "-k" and "--deselect" work as expected with defopts="auto"

chmp commented 2 years ago

@axil I think I found a nice compromise, that fits nicely with the remaining design of ipytest: "{MODULE}::test_1" can be shortened to {test_1}. This shorthand can be used both with ipytest.run and the magics (see the "Selecting tests" section in the Example notebooks).

Now your initial example can be written as:

ipytest.run('{test1}')

You can test it by installing pip install ipytest==0.13.0a2.

axil commented 2 years ago

Yes, indeed, it is better from the point of view of the number of keystrokes, but usually the thing inside the curly braces is something that is being expanded. And here 'test1' looks like the result of the expansion already. As for me It's a bit anti-intuive from the first glance. I would give it yet another thought. :)

axil commented 2 years ago

As for me the syntax of pytest execution (the main function) is the least convenient feature of pytest in general. The only thing it reflects is that pytest author(s) prefer calling it from the command line. Calling it programmatically is just a stub. It definitely needs improvement and does not receive it – most probably just to keep backwards compatibility. In ipytest you are not bound by such considerations and can choose the most convenient form.

axil commented 2 years ago

The original pytest signature is

pytest [options] [file_or_dir] [file_or_dir] [...]

What are the chances that someone will test a different file or directory from an ipynb file? I think that it would be great if minimal example for ipytest looked like

import ipytest

def test1():
    assert 2*2 == 5

ipytest.run('test1')

and it could be run from a single cell. What are the downsides of such syntax?

For jupyter notebook specifying functions by name is more essential than in plain python files, because if you rename a test function the old function objects still sits in memory and if you call ipytest.run() it launches both new and old versions.

chmp commented 2 years ago

While I generally agree with your sentinment, breaking compatibility with pytest (or the main entry points of ipytest) is a no-go for me. Supporting as much of pytest as possible is one of the major design goals of this package. Anything that ipytest offers should not interfere with pytest.

What are the chances that someone will test a different file or directory from an ipynb file?

Actually, I do it from time to time. As I use ipytest to prototype code that I then shift into modules. In that case, I have tests split between files and the notebook.

In ipytest you are not bound by such considerations and can choose the most convenient form.

Some folks are using ipytest to teach pytest, they would run into problems, if ipytest broke compatibility.

I'm closing this issue for now, as your use case can now be handled much more conviently than before.

However, I believe ipytest should offer you all the means to write you own wrapper with an interface that better suits your use case. If not feel free to open another issue. I am open to shift the internals around or expose more of the underlying machinery.

For example you could write your own wrapper with as little code as:

def run_tests(*tests):
  args = []
  for test in tests:
     name = str(test) if not hasattr(test, '__name__') else test.__name__
     args.append("{" + name + "}")

  ipytest.run(*args)

run_tests("test_example1", test_example2)
axil commented 2 years ago

breaking compatibility with pytest (or the main entry points of ipytest) is a no-go for me.

+1

Supporting as much of pytest as possible is one of the major design goals of this package.

+1

Anything that ipytest offers should not interfere with pytest.

+1

Yes, it all sounds reasonable. Yet run instead of main and defopts keyword are deviations from the original pytest, supposedly intended to make the life of the end user easier. Why not make another step in this direction and allow something like

ipytest.run('::test1') 

Here the leading double colon is automatically prepended with {MODULE} and 'defopts' is automatically turned on. This syntax: – keeps backwards compatibility, – is closer to the original pytest syntax and – has the same number of keystrokes as run_tests('{test1}') – doesn't bring up the legitimate question "I didn't think I have a variable named "test1" to expand, where has it come from?" of the curly braces syntax.

PS

much more conviently than before.

Having a token inside curly braces that is not a variable or an expression is not pythonic in my opinion (although this is very subjective, of course).

Some folks are using ipytest to teach pytest, they would run into problems, if ipytest broke compatibility.

That's what I'm going to do next semester, too — I mean teaching, not running in to problems, hopefully :)

chmp commented 2 years ago

That's what I'm going to do next semester, too — I mean teaching, not running in to problems, hopefully :)

Haha. If you do and ipytest is to blame, please let me know so I can attempt to remedy that :D

Here the leading double colon is automatically prepended with {MODULE} and 'defopts' is automatically turned on.

TBH, I like this much more, than the current setup. I thought about it, too. The issue here is, that there is no way for me distinguish between a node id and any other argument starting with "::" without reimplementing the pytest argument parsing logic (and that of any plugin as well) (there is a minimal parser for defopts="auto", but if that breaks, you can always overwrite it). Since pytest is not really designed to be used programmatically, parsing the arguments properly seems like a loosing battle.

For example:

%%ipytest --deselect={test_foo}
%%ipytest --deselect {test_foo}
%%ipytest {test_bar}

work all as expected. I think that all the places node ids can appear in plain pytest, but who knows what plugins are doing and wether in all places where "::" appears a node id is expected?

I also think the the interpolation syntax is plus here: it is so foreign, there is little danger of colliding with pytest argument syntax. And even if it does, there is a well defined way of escaping by using {{ instead of {. How should the double colon be escaped?

Finally, using the {MODULE} already reserved the braces as a special construct. Maybe I didn't think it through enough, but now I need a very good reason to break backwards compatibility.

Having a token inside curly braces that is not a variable or an expression is not pythonic in my opinion (although this is very subjective, of course).

You could argue, that all it is doing is using an implicit mapping of function name to node id ;) But I know what you mean.

Yet run instead of main and defopts keyword are deviations from the original pytest,

Actually renaming run into main (or at least offering it as an alias) could be a nice addition. The ipytest.run option was always somewhat of an afterhought, as I almost exclusively use the magics.

As for adding the defopts argument to run: I thinking about using a ipytest_config argument that expects a dictionary of options to minimize the breakage of the pytest interface. That option though seemed awfully unweidly. I am also still thinking about removing the arguments alltogether before relasing the new version and have people rely on setting the config manually (at least your use case is well enough handled by defopts="auto" in my eyes).

axil commented 2 years ago

What is defopts="auto"? I haven't found it in the docs.

I personally think curly braces are much more likely to be appear in plugin strings than :: (though I don't have any examples for either of them so it's not that important).

Re-using the curly braces (in addition to being unpythonic) is inconsistent with how you used them before:

Did you try exposing MODULE as __file__ in ipytest.autoconfig?

I would rename run to main, made it 100% compatible with pytest.main (for example, args instead of *args) and create a run function (or something similar) with a pythonic interface:

run('test1', 'test2', '-test3', contains='this and that', durations=10, exitfirst=True)

It could properly prepend MODULE to method names, translate leading '-' to --deselect, 'contains' (or 'k'?) to -k, other keyword arguments to themselves, etc.

Also distinction between magics and non-magics should be clarified in the docs. For example:

Maybe it is possible to autoforget the functions from non-magic cells as well? Did you try it?

There're issues with the documentation of run: argument filename described, but is missing in the provided signature, formatting of double underscores.

Also here is an interesting project that I think is worthy of mentioning in the 'related packages': https://pytest-exploratory.readthedocs.io/

PS I'm writing all of this because I'm working on a wrapper of (another) library myself and I know how valuable a discussion can be :)

chmp commented 2 years ago

if in future you decide to add another variable in addition to MODULE, you'll not be able to do it,

I already reserved all uppercase symbols ;) If you use an all upercase testname with { ... }, it will raise a KeyError. See for example here

Did you try exposing MODULE as __file__ in ipytest.autoconfig?

There was the option to set __file__ in the notebook scope, similar to what is available in normal files, but it was quite brittle and error prone to use, hence I removed it again. Using __file__ as an keyword argument feels very unpythonic.

Maybe it is possible to autoforget the functions from non-magic cells as well? Did you try it?

I would not know how without context managers or non-local stuff.

There're issues with the documentation of run: argument filename described, but is missing in the provided signature, formatting of double underscores.

It's already fixed on develop. There you also find the documentation for the currently unreleased version

Also here is an interesting project that I think is worthy of mentioning in the 'related packages': https://pytest-exploratory.readthedocs.io/

Thanks. Nice find. I will add it

axil commented 2 years ago

I would not know how without context managers or non-local stuff.

It might be possible through registering a 'post_run_cell' callback. In this callback: – annotate functions and classes with an flag upon creation in 'post_run_cell' eg test1._ipytest_seen = True through scanning the globals() – keep a registry of which cell declared which function – in the subsequent 'post_run_cell' check if the list of the unannotated functions does not match the list of functions previously declared in this cell as recorded in the registry; delete the unmatched functions. – upon the deletion of a cell delete functions declared in this cell.

It might be tricky and not worth the while if the magics already do the job well enough, but in general it looks feasible to me.

chmp commented 2 years ago

Damn. Every time I think, but surely now I have seen all there is to see for IPython, there so something new. I was completely unware, that there are these hooks (for future ref: the docs). Thanks for making me aware. That would indeed be an option.