Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

importnb causes pytest to run tests twice #160

Open
ddouglas87 opened this issue Oct 18, 2024 · 4 comments
Open

importnb causes pytest to run tests twice #160

ddouglas87 opened this issue Oct 18, 2024 · 4 comments

Comments

@ddouglas87
Copy link

ddouglas87 commented Oct 18, 2024

When I pip install importnb individual pytests run twice.

This is quite weird behavior, because importing importnb isn't required. All you have to do is pip install it and tests start running twice. When uninstalling importnb the issue goes away.

To reproduce:

cd /tmp/
mkdir project
cd project/
python3 -m venv .venv
source .venv/bin/activate
pip install pytest
mkdir tests
cd tests/
vim test.py

def test_empty():
    ...

pytest test.py -k test_empty
================================================ test session starts =================================================
platform linux -- Python 3.11.10, pytest-8.3.3, pluggy-1.5.0
rootdir: /tmp/project/tests
collected 1 item                                                                                                     

test.py .                                                                                                      [100%]

================================================= 1 passed in 0.00s ==================================================

pip install importnb

pytest test.py -k test_empty
================================================ test session starts =================================================
platform linux -- Python 3.11.10, pytest-8.3.3, pluggy-1.5.0
rootdir: /tmp/project/tests
plugins: importnb-2023.11.1
collected 2 items                                                                                                    

test.py ..                                                                                                     [ 50%]

================================================= 2 passed in 0.01s ==================================================

Note: I've tested this with Python 3.11, 3.12, and 3.13. I've also tested it with pytest 8.3 and 8.2. This is reproducible both in PyCharm and in the command line.

Also, it doesn't work with the current version from github:

pytest test.py -k test_empty
================================================ test session starts =================================================
platform linux -- Python 3.11.10, pytest-8.3.3, pluggy-1.5.0
rootdir: /tmp/project/tests
plugins: importnb-2023.11.2.dev11+ga633de3
collected 2 items                                                                                                    

test.py ..                                                                                                     [ 50%]

================================================= 2 passed in 0.01s ==================================================
@ddouglas87
Copy link
Author

A temporary fix for this problem is to disable the importnb pytest plugin until it is fixed. This comment explains how. I chose to add the setting into my pyproject.toml file at the root of my project directory. Here's what my pyproject.toml file looks like:

[tool.pytest.ini_options]
addopts = [
    "--capture=no",
    "--log-cli-level=10",
    "-p no:importnb",
]

The capture=no and log level are so print statements can be seen while running a test. The "-p no:importnb", line fixes the issue.

@tonyfast
Copy link
Member

thanks for trying importnb and raising this issue. i think a solution is make testing notebook with importnb not the default and then dealing with the duplicate tests.

as a user, i feel like you would want to opt into the testing. is this what you'd expect?

Also, it doesn't work with the current version from github:

are you saying the current code isn't discovering the tests.

@ddouglas87
Copy link
Author

ddouglas87 commented Oct 28, 2024

Also, it doesn't work with the current version from github:

are you saying the current code isn't discovering the tests.

The most up to date version from github still has the bug in it, is what I meant to say. You can see the example output right below it that shows two tests when there should be one.

as a user, i feel like you would want to opt into the testing. is this what you'd expect?

I prefer "it just works". Running tests twice is definitely a bug. As an end user I don't care under the hood how it works, via opting in or not opting in, I want it to just work. If opting in means it creates a bug, that is still an issue.

I don't know if this helps, but here's how I run tests: My preference is to have a separate tests folder with their own .py files filled with tests, not to have tests in my main code base. Two reasons to do this is: 1) Tests can double as documentation. It's easier to see all the tests together when using them this way. 2) Tests can import libraries the main project does not. I have a separate requirements.txt file just for tests.

My philosophy is to import a notebook like it is a .py file. I treat an .ipynb file just like a .py file. Because of this, if I'm going to write tests for a notebook, I'm putting the tests in a separate .py file in a tests folder. I will not put tests directly in the notebook itself.


P.S. importnb is an AWESOME library. Ever since the functionality was added to only import functions by doing with Notebook(include_non_defs=False, no_magic=True):, it's been incredibly useful.

It's sad this library is not well known and not well supported. imo this should be an industry practice to do it this way, but because it's so unknown, few people even know it's an option to productionize models this way. The other import notebook python libraries work differently and imo aren't as good.

@tonyfast
Copy link
Member

The most up to date version from github still has the bug in it, is what I meant to say. You can see the example output right below it that shows two tests when there should be one.

got it. i'll take a look into the test duplication .

i'll likely make importnb's pytest an opt-in feature. the original philosophy was that notebooks are used for testing ideas, so we should promote formalizing them as tests sooner. i don't agree as much anymore. the primary goal should be to import notebooks. testing is an extra feature.

P.S. importnb is an AWESOME library. Ever since the functionality was added to only import functions by doing with Notebook(include_non_defs=False, no_magic=True):, it's been incredibly useful.

hell yea. thanks for these kind words. i'm really glad those features have been useful for you. let us know if there are any public repos you are using importnb in. it'd be cool to see it in practice.

imo this should be an industry practice to do it this way, but because it's so unknown, few people even know it's an option to productionize models this way.

its cool you've seen the light. hopefully, our stoic optimism pays off and folks become more accepting of notebooks as a source code. there is a costly redundant work in translating notebooks to python that probably doesn't need to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants