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

👷‍♂️ Ensure the PYTHONPATH is set properly when testing the tutorial scripts #407

Merged

Conversation

theMarix
Copy link
Contributor

When packaging Typer for openSUSE I ran into errors because the tutorial
scripts were unable to import their colocated modules. Curiously this
only seems to be occurring when these scripts are run via coverage, as
they are in the tests. Them being run via coverage however also prevents
just changing the working directory for the script runs, as then the
coverage file would end up in the wrong directory.

Curiously, I have not been able to reproduce this issue on openSUSE Leap
but only seen it on openSUSE Tumbleweed. Thus, there might be something
weird with the Python stack or the coverage version on Tumbleweed.
However, as the same PYTHONPATH-patching is also done for the tests of
the tutorial code that run it directly and not as a subprocess, I think
it is just consistent to also do this for the script test. Thus, please view this PR
as a suggestion that improves consistency. If you think it does not provide
value outside of my very specific use-case, I am completely fine with it being
declined.

For reference, this is the error that I am observing in the packaging
environment and that gets resolved by this commit:

[ 123s] =================================== FAILURES ===================================
[ 123s] _________________________________ test_scripts _________________________________
[ 123s]
[ 123s] mod = <module 'docs_src.subcommands.tutorial001.main' from '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py'>
[ 123s]
[ 123s] def test_scripts(mod):
[ 123s] from docs_src.subcommands.tutorial001 import items, users
[ 123s]
[ 123s] for module in [mod, items, users]:
[ 123s] result = subprocess.run(
[ 123s] ["coverage", "run", module.file, "--help"],
[ 123s] stdout=subprocess.PIPE,
[ 123s] stderr=subprocess.PIPE,
[ 123s] encoding="utf-8",
[ 123s] )
[ 123s] > assert "Usage" in result.stdout
[ 123s] E assert 'Usage' in ''
[ 123s] E + where '' = CompletedProcess(args=['coverage', 'run', '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py', '--help'], returncode=1, stdout='', stderr='Traceback (most recent call last):\n File "/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py", line 3, in \n import items\nModuleNotFoundError: No module named 'items'\n').stdout
[ 123s]
[ 123s] tests/test_tutorial/test_subcommands/test_tutorial001.py:94: AssertionError
[ 123s] _________________________________ test_scripts _________________________________
[ 123s]
[ 123s] mod = <module 'docs_src.subcommands.tutorial003.main' from '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py'>
[ 123s]
[ 123s] def test_scripts(mod):
[ 123s] from docs_src.subcommands.tutorial003 import items, lands, reigns, towns, users
[ 123s]
[ 123s] for module in [mod, items, lands, reigns, towns, users]:
[ 123s] result = subprocess.run(
[ 123s] ["coverage", "run", module.file, "--help"],
[ 123s] stdout=subprocess.PIPE,
[ 123s] stderr=subprocess.PIPE,
[ 123s] encoding="utf-8",
[ 123s] )
[ 123s] > assert "Usage" in result.stdout
[ 123s] E assert 'Usage' in ''
[ 123s] E + where '' = CompletedProcess(args=['coverage', 'run', '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py', '--help'], returncode=1, stdout='', stderr='Traceback (most recent call last):\n File "/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py", line 3, in \n import items\nModuleNotFoundError: No module named 'items'\n').stdout
[ 123s]
[ 123s] tests/test_tutorial/test_subcommands/test_tutorial003.py:146: AssertionError

@theMarix theMarix force-pushed the set-pythonpath-for-tutorial-script-tests branch from da489a1 to e575de2 Compare July 18, 2022 10:18
@tiangolo
Copy link
Member

tiangolo commented Nov 5, 2022

Thanks for the contribution and interest!

I just updated a bunch of dependencies and fought a lot related things with coverage, pytest-cov, etc 😅

Could you check if the current version works correctly?

I also have a question, pure curiosity, why would OpenSUSE re-package Typer? What is the benefit of providing it as a distro package instead of a simple Python package installed with pip?

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

📝 Docs preview for commit e4599f2 at: https://636674af39a2407818ea75d6--typertiangolo.netlify.app

When packaging Typer for openSUSE I ran into errors because the tutorial
scripts were unable to import their colocated modules. Curiously this
only seems to be occurring when these scripts are run via coverage, as
they are in the tests. Them being run via coverage however also prevents
just changing the working directory for the script runs, as then the
coverage file would end up in the wrong directory.

Curiously, I have not been able to reproduce this issue on openSUSE Leap
but only seen it on openSUSE Tumbleweed. Thus, there might be something
weird with the Python stack or the coverage version on Tumbleweed.
However, as the same PYTHONPATH-patching is also done for the tests of
the tutorial code that run it directly and not as a subprocess, I think
it is just consistent to also do this for the script test.

For reference, this is the error that I am observing in the packaging
environment and that gets resolved by this commit:

[  123s] =================================== FAILURES ===================================
[  123s] _________________________________ test_scripts _________________________________
[  123s]
[  123s] mod = <module 'docs_src.subcommands.tutorial001.main' from '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py'>
[  123s]
[  123s]     def test_scripts(mod):
[  123s]         from docs_src.subcommands.tutorial001 import items, users
[  123s]
[  123s]         for module in [mod, items, users]:
[  123s]             result = subprocess.run(
[  123s]                 ["coverage", "run", module.__file__, "--help"],
[  123s]                 stdout=subprocess.PIPE,
[  123s]                 stderr=subprocess.PIPE,
[  123s]                 encoding="utf-8",
[  123s]             )
[  123s] >           assert "Usage" in result.stdout
[  123s] E           assert 'Usage' in ''
[  123s] E            +  where '' = CompletedProcess(args=['coverage', 'run', '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py', '--help'], returncode=1, stdout='', stderr='Traceback (most recent call last):\n  File "/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial001/main.py", line 3, in <module>\n    import items\nModuleNotFoundError: No module named \'items\'\n').stdout
[  123s]
[  123s] tests/test_tutorial/test_subcommands/test_tutorial001.py:94: AssertionError
[  123s] _________________________________ test_scripts _________________________________
[  123s]
[  123s] mod = <module 'docs_src.subcommands.tutorial003.main' from '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py'>
[  123s]
[  123s]     def test_scripts(mod):
[  123s]         from docs_src.subcommands.tutorial003 import items, lands, reigns, towns, users
[  123s]
[  123s]         for module in [mod, items, lands, reigns, towns, users]:
[  123s]             result = subprocess.run(
[  123s]                 ["coverage", "run", module.__file__, "--help"],
[  123s]                 stdout=subprocess.PIPE,
[  123s]                 stderr=subprocess.PIPE,
[  123s]                 encoding="utf-8",
[  123s]             )
[  123s] >           assert "Usage" in result.stdout
[  123s] E           assert 'Usage' in ''
[  123s] E            +  where '' = CompletedProcess(args=['coverage', 'run', '/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py', '--help'], returncode=1, stdout='', stderr='Traceback (most recent call last):\n  File "/home/abuild/rpmbuild/BUILD/typer-0.4.1/docs_src/subcommands/tutorial003/main.py", line 3, in <module>\n    import items\nModuleNotFoundError: No module named \'items\'\n').stdout
[  123s]
[  123s] tests/test_tutorial/test_subcommands/test_tutorial003.py:146: AssertionError
@theMarix theMarix force-pushed the set-pythonpath-for-tutorial-script-tests branch from e4599f2 to 8e4c716 Compare November 6, 2022 19:37
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

📝 Docs preview for commit 8e4c716 at: https://63680d0ba9dce9131dec4c58--typertiangolo.netlify.app

@theMarix
Copy link
Contributor Author

theMarix commented Nov 6, 2022

Yes, this still works. I rebased onto latest master make this explicit.

Regarding your question as to why one would repackage this within a distribution. You are right. In a world of developers or custom applications where you install via pip this does not make sense. And I also would not expect anybody to ever directly do zypper install python311-typer. But if you package applications that only happen to be written in Python and the user won't be aware of that but just wants to get the app, that app needs to be able to pull in the module via system packaging so that you can do zypper install cool-app-I-want.

@github-actions github-actions bot removed the answered label Nov 6, 2022
@tiangolo
Copy link
Member

tiangolo commented Nov 7, 2022

Ah, get it, thanks for the explanation!

And now, after rebasing from master, is this change still needed?

@theMarix
Copy link
Contributor Author

theMarix commented Nov 7, 2022

Yes, I still need this patch. It also seems I now triggered this on CI in #408 so it's no longer exclusive to openSUSE Tumblweed.

@tiangolo tiangolo changed the title Ensure the PYTHONPATH is set properly when testing the tutorial scripts 👷‍♂️ Ensure the PYTHONPATH is set properly when testing the tutorial scripts Nov 7, 2022
@tiangolo
Copy link
Member

tiangolo commented Nov 7, 2022

Cool, let's do it then, thanks! 🚀

@tiangolo tiangolo merged commit 52f488d into fastapi:master Nov 7, 2022
@theMarix theMarix deleted the set-pythonpath-for-tutorial-script-tests branch November 7, 2022 20:49
alexreg pushed a commit to alexreg/typer-cloup that referenced this pull request Nov 11, 2022
alexreg pushed a commit to alexreg/typer-cloup that referenced this pull request Nov 15, 2022
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

Successfully merging this pull request may close these issues.

2 participants