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

tests are shipped as a module with the package #140

Closed
fpgmaas opened this issue Mar 14, 2025 · 6 comments · Fixed by #139
Closed

tests are shipped as a module with the package #140

fpgmaas opened this issue Mar 14, 2025 · 6 comments · Fixed by #139

Comments

@fpgmaas
Copy link

fpgmaas commented Mar 14, 2025

What is the problem?

tests directory is shipped as part of the package. This can be a problem, since many projects contain a tests directory themselves. For example, this can be a problem for projects that import from their tests directory while running the tests.
The reason for this is that the module tests is now ambiguous; it refers both to tests in the virtual environment, and to the tests directory in their project. This will lead to errors while running the unit tests like:

ModuleNotFoundError: No module named 'tests.utils'

The issue can be confirmed with the following steps:

pip download --no-deps --only-binary=:all: msal-extensions==1.3.0
unzip msal_extensions-1.3.0-py3-none-any.whl -d extracted_wheel
ls -1 extracted_wheel 

This shows the following directories exist in the wheel file:

msal_extensions
msal_extensions-1.3.0.dist-info 
tests

How to solve this?

tests should not be shipped with the msal-extensions package. Probably the line packages=find_packages() in setup.py should be modified.

@fpgmaas fpgmaas changed the title tests are shipped as. module with the package tests are shipped as a module with the package Mar 14, 2025
@fpgmaas
Copy link
Author

fpgmaas commented Mar 14, 2025

Opened a PR that I think should fix this here.

@rayluo
Copy link
Contributor

rayluo commented Mar 14, 2025

Thanks for the catch. I intend to merge your PR #139 for cleaning purpose, if nothing else.

Out of curiosity, how bad is it for the shipped MSAL EX 1.3.0, though? Do we have to ship this fix quick? What actual issue did you run into, @fpgmaas , @mpkuth? I did some quick tests, and it seems a project can still pick up their own tests folder.

py311demo@antix1:/tmp$ python -c "import tests as t; print(t)"
<module 'tests' from '/tmp/tests/__init__.py'>

py311demo@antix1:/tmp$ cd dir_without_tests
/tmp/dir_without_tests
py311demo@antix1:/tmp/dir_without_tests$ python -c "import tests as t; print(t)"
<module 'tests' from '/home/demo/Live-usb-storage/repo/microsoft-authentication-extensions-for-python/.tox/py311/lib/python3.11/site-packages/tests/__init__.py'>

UPDATE:

The example in my tests above used a tests folder with a __init__.py file, and the import was able to locate it first, regardless of whether a package tests exists in site-packages. But if I remove that __init__.py file, the import indeed hits the package tests exists in site-packages. This subtle import behavior difference is likely the root cause.

py311demo@antix1:/tmp$ rm tests/__init__.py 
py311demo@antix1:/tmp$ python -c "import tests as t; print(t)"
<module 'tests' from '/home/demo/Live-usb-storage/repo/microsoft-authentication-extensions-for-python/.tox/py311/lib/python3.11/site-packages/tests/__init__.py'>

@mpkuth rightfully pointed out that the __init__.py-less tests folder is likely common.

In any case, the tests will be removed from msal-extensions' next release, coming soon.

@fpgmaas
Copy link
Author

fpgmaas commented Mar 14, 2025

@rayluo For some of my projects it breaks the CI/CD pipelines. I created a small reproducible example here. The unit tests succeed without msal-extensions, but they fail when msal-extensions is added as a dependency.

There are some workarounds of course;

  • I can pin msal-extensions to 1.2.0 since the latest version of azure-identity depends on >=1.2.0
  • move tests.utils to test_utils.

So it is not very urgent, but I still think a 1.3.1 release would be nice to release quickly in this case to prevent users of the package from having to modify their projects.

@mpkuth
Copy link

mpkuth commented Mar 14, 2025

We use the same structure as the example provided by @fpgmaas. The update caused pytest to fail with

ImportError: cannot import name 'utils' from 'tests'
(<path>/site-packages/x64/prod/tests/__init__.py)

Eventually we tracked down the wayward tests folder in site-packages to this package based on its contents.

We worked around it by adding __init__.py to our tests folder and changing the imports

-from tests import utils
+from . import utils

The layout we use is suggested in the pytest docs, so I think this change has the potential to affect a lot of folks.
https://docs.pytest.org/en/latest/explanation/goodpractices.html#tests-outside-application-code

@rayluo
Copy link
Contributor

rayluo commented Mar 14, 2025

Thanks @fpgmaas for going extra mile to set up a repo for repro. And thanks @mpkuth for rightfully pointed out that the __init__.py-less tests folder is likely common. I have also appended more test findings into my previous message above.

The tests will be removed from msal-extensions' next release, coming soon.

@rayluo
Copy link
Contributor

rayluo commented Mar 14, 2025

Version 1.3.1 is shipped.

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 a pull request may close this issue.

3 participants