-
Notifications
You must be signed in to change notification settings - Fork 79
tests: consolidate editable and isolated fixtures #1194
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
Conversation
bbd1934 to
ccde78a
Compare
|
Ahhh, the tests are failing at too long paths? Well that's really annoying. c094c95 should alleviate this issue for a bit, although it makes the path less navigable. |
a12c12e to
c094c95
Compare
| @pytest.fixture | ||
| def package_simple_pyproject_source_dir( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| def package( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So this is now not just a factory for a PackageInfo, but takes care of staging the named package sources into a fresh temporary directory and changing into that directory, which imposes the normalized package layout that motivates all of the file moves. Test functions only need to capture the fixture as an argument if they want to check the hashes. I wish I could think of an alternative name to suggest. This is more readable, overall, I think. The parameterization is not intuitive, but the examples are clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could think of an alternative name to suggest.
Always welcome if you have naming candidates.
Test functions only need to capture the fixture as an argument if they want to check the hashes
Could expose some functionality for it to avoid needing to ignore the fixture in parameters, just haven't found a worthy one to do so with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR consolidates test fixtures for package setup, editable installs, and build isolation into parameterized fixtures, reducing code duplication and improving test maintainability in preparation for #1193.
- Introduced centralized
package,multiple_packages,isolate, andeditablefixtures with parametrization support - Migrated all test files to use the new parametrized fixture approach instead of individual package fixtures
- Added comprehensive
importlib_editabletest package to support more extensive editable installation testing
Reviewed changes
Copilot reviewed 14 out of 43 changed files in this pull request and generated 40 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/conftest.py | Core fixture refactoring: added parametrized package, multiple_packages, isolate, and editable fixtures; removed individual package fixtures; introduced Isolate and Editable dataclasses; updated process_package signature |
| tests/test_setuptools_pep518.py | Migrated to use parametrized package fixture instead of package_simple_setuptools_ext |
| tests/test_setuptools_pep517.py | Migrated to use parametrized package fixture for simple_setuptools_ext, toml_setuptools_ext, and mixed_setuptools tests |
| tests/test_pyproject_purelib.py | Migrated to use parametrized package fixture instead of package_simple_purelib_package |
| tests/test_pyproject_pep660.py | Removed local editable_mode fixture; migrated to use centralized parametrized editable fixture and removed typing import |
| tests/test_pyproject_pep518.py | Migrated to use parametrized package fixture instead of package_sdist_config |
| tests/test_pyproject_pep517.py | Migrated to use parametrized package fixture for multiple test packages (simple_pyproject_script_with_flags, simple_pyproject_source_dir, pep639_pure) |
| tests/test_pyproject_extra_dirs.py | Migrated to use parametrized package fixture instead of package_filepath_pure |
| tests/test_prepare_metadata.py | Migrated to use parametrized package fixture instead of package_simplest_c |
| tests/test_hatchling.py | Migrated to use parametrized package fixture instead of package_hatchling |
| tests/test_editable.py | Major refactoring: removed helper function _setup_package_for_editable_layout_tests; migrated to use centralized isolate, editable, and multiple_packages fixtures; removed now-unused conftest imports |
| tests/test_dynamic_metadata.py | Migrated all dynamic_metadata tests to use parametrized package fixture |
| tests/test_broken_fallback.py | Migrated to use parametrized package fixture instead of broken_fallback fixture |
| tests/packages/importlib_editable/* | Added comprehensive new test package with nested subpackages, extension modules, and pure Python modules to test editable installation import behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_setuptools_pep517.py
Outdated
| @pytest.mark.compile | ||
| @pytest.mark.configure | ||
| @pytest.mark.usefixtures("package_mixed_setuptools") | ||
| @pytest.mark.parametrize("package", {"mixed_setuptools"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"mixed_setuptools"} instead of the conventional list ["mixed_setuptools"] or tuple ("mixed_setuptools",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_pyproject_pep660.py
Outdated
| ) | ||
| @pytest.mark.usefixtures("package_simplest_c") | ||
| def test_pep660_wheel(editable_mode: str, tmp_path: Path): | ||
| @pytest.mark.parametrize("package", {"simplest_c"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"simplest_c"} instead of the conventional list ["simplest_c"] or tuple ("simplest_c",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_pyproject_pep517.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("package_pep639_pure") | ||
| @pytest.mark.parametrize("package", {"pep639_pure"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"pep639_pure"} instead of the conventional list ["pep639_pure"] or tuple ("pep639_pure",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_editable.py
Outdated
| ], | ||
| ) | ||
| @pytest.mark.usefixtures("navigate_editable") | ||
| @pytest.mark.parametrize("package", {"navigate_editable"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"navigate_editable"} instead of the conventional list ["navigate_editable"] or tuple ("navigate_editable",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_dynamic_metadata.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("package_dynamic_metadata") | ||
| @pytest.mark.parametrize("package", {"dynamic_metadata"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"dynamic_metadata"} instead of the conventional list ["dynamic_metadata"] or tuple ("dynamic_metadata",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_broken_fallback.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("broken_fallback") | ||
| @pytest.mark.parametrize("package", {"broken_fallback"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"broken_fallback"} instead of the conventional list ["broken_fallback"] or tuple ("broken_fallback",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_pyproject_pep517.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("package_pep639_pure") | ||
| @pytest.mark.parametrize("package", {"pep639_pure"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"pep639_pure"} instead of the conventional list ["pep639_pure"] or tuple ("pep639_pure",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_prepare_metadata.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("package_simplest_c") | ||
| @pytest.mark.parametrize("package", {"simplest_c"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"simplest_c"} instead of the conventional list ["simplest_c"] or tuple ("simplest_c",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_broken_fallback.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("broken_fallback") | ||
| @pytest.mark.parametrize("package", {"broken_fallback"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"broken_fallback"} instead of the conventional list ["broken_fallback"] or tuple ("broken_fallback",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
tests/test_setuptools_pep517.py
Outdated
|
|
||
|
|
||
| @pytest.mark.usefixtures("package_toml_setuptools_ext") | ||
| @pytest.mark.parametrize("package", {"toml_setuptools_ext"}, indirect=True) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parametrize decorator uses a set {"toml_setuptools_ext"} instead of the conventional list ["toml_setuptools_ext"] or tuple ("toml_setuptools_ext",). While sets are iterable and this technically works, it's non-idiomatic pytest style and can be confusing. Sets are unordered, which could lead to non-deterministic test ordering if multiple values were present. Consider using a list or tuple instead.
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
92f0fd3 to
6fac3c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 43 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A bit of cleanup in preparation for #1193