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

Implicit namespaces support #165

Merged
merged 5 commits into from Feb 27, 2024
Merged

Implicit namespaces support #165

merged 5 commits into from Feb 27, 2024

Conversation

gforcada
Copy link
Contributor

Closes #160

Allow zope.testrunner to find tests suites for packages that follow the PEP 420 implicit namespaces.

Note that this works on test packages, like zopefoundation/megrok.strictrequire#8

Test suite needs to be adjusted, but at least we have a reference implementation that seems to be working for packages already using implicit namespaces.

To test it with megrok.strictrequire:

git clone git@github.com:zopefoundation/zope.testrunner
cd zope.testrunner
git checkout implicit-namespaces
git clone git@github.com:zopefoundation/megrok.strictrequire
cd megrok.strictrequire
git checkout pep440
cd ..
python3.11 -m venv venv
. venv/bin/activate
pip install -e megrok.strictrequire[test]
pip install -e .
zope-testrunner --test-path megrok.strictrequire/src

Most of the test failures of zope.testrunner's own test suite is due to, probably, the test runner finding too much tests. I will dig into it later, but I wanted to post the PR as soon as it was minimally working ✨

This specific part of this doctest was hanging wit the implicit
namespace support.

Handling `sdtin` as with the other parts of this doctest makes it at
least not hang.
Seems that this file was irrelevant for the test suite, and only when
the implicit namespace support was added, the file was found and was
breaking the test suite.
@gforcada
Copy link
Contributor Author

Status update: all tests pass and the only two adjustments on the tests seem to be fine enough.

I could move those tests adjustments to a separate PR to validate that, indeed, they are not needed for this PR to pass, but stand on their own.

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only side effect is, it may take longer to discover tests, right? But I think this is fine. If someone complains, testrunner can be configured to search more specific.

@dataflake
Copy link
Member

Test suite needs to be adjusted, but at least we have a reference implementation that seems to be working for packages already using implicit namespaces.

What do you mean by "test suite needs to be adjusted"? Do you mean existing packages need their tests changed?

@jensens
Copy link
Member

jensens commented Feb 20, 2024

What do you mean by "test suite needs to be adjusted"? Do you mean existing packages need their tests changed?

Since it did not traverse into non-__init__.py-containing directories and now does, it may result in longer discovery runs. To avoid this we may want to configure the zope.testrunner to search more precisely using other --path and/or --test-pattern configurations.
OTOH I do not expect it to discover tests, which it had not found before - so, it won't break.

@gforcada
Copy link
Contributor Author

@dataflake sorry, that was an interim comment, when I first pushed this PR, I knew that the code was working, as I used the megrok.staticrequire distribution as a test sample, but I was not sure about zope.testrunner's own test suite.

I was expecting more breakage, but turned out not to be 👍🏾

Regarding test discovery, I see a problem, like we faced in z3c.dependencychecker when we added support for implicit namespaces (reinout/z3c.dependencychecker#126) and the two follow up PRs to improve the time it takes to run: reinout/z3c.dependencychecker#128

Basically, ignore folders with leading dots, __pycache__ or node_modules for example.

@dataflake
Copy link
Member

Basically, ignore folders with leading dots, __pycache__ or node_modules for example.

It looks like the current code here only omits leading dot folders with the regular expression identifier:

identifier = re.compile(r'[_a-zA-Z]\w*$').match

This expression is used to filter out folders while walking the tree:

dirs[:] = [d for d in dirs if identifier(d)]

I suggest adding at least __pycache__ to the undesirable folder names there.

@davisagli
Copy link
Member

I'm not sure what would be the simplest way to implement this, but it would be nice if the test discovery skipped any paths that are matched by .gitignore configuration.

@icemac
Copy link
Member

icemac commented Feb 21, 2024

I'm not sure what would be the simplest way to implement this, but it would be nice if the test discovery skipped any paths that are matched by .gitignore configuration.

There is https://github.com/thehanimo/py-gitignore under MIT license which seems to be capable of checking whether a path is ignored via .gitignore.

@d-maurer
Copy link
Contributor

d-maurer commented Feb 21, 2024 via email

@dataflake
Copy link
Member

I agree with Dieter and I don't think there's much gain for the price of yet another dependency or manually written code that needs maintaining going forward. My suggestion still stands, expand the current exclusion of dot folders by adding __pycache__ and get on with this.

@davisagli
Copy link
Member

@d-maurer Good point, I agree that's a compelling reason to not use .gitignore.

@gforcada
Copy link
Contributor Author

I adjusted the code just slightly to ignore well known folders we want to skip.

I added some minimal tests to ensure we know what we are accepting/rejecting right now.

🍀

@jensens
Copy link
Member

jensens commented Feb 27, 2024

As an idea for a future iteration as a new PR (to get this one here merged soon): Get the IGNORE_FOLDERS configuration from a custom section in pyproject.toml?

@gforcada
Copy link
Contributor Author

@jensens yes, I thought about that as well, that one way or another users could provide their own list of ignored folders 👍🏾

But yes, let's merge this, a new release, and see how much it breaks 🍀 😅

@gforcada gforcada merged commit 4e98a1e into master Feb 27, 2024
39 checks passed
@gforcada gforcada deleted the implicit-namespaces branch February 27, 2024 09:26
@dataflake
Copy link
Member

Release 6.4 is now published

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.

Support implicit namespaces (PEP 420)
6 participants