Skip to content

Conversation

@rossbar
Copy link
Contributor

@rossbar rossbar commented Dec 2, 2025

Expect this initial commit to fail - verifying that the tests work as expected.

@rossbar
Copy link
Contributor Author

rossbar commented Dec 2, 2025

This recipe behaves as expected (i.e. fails) locally, but something wonky is going on with the actions (not sure why they're not running).

Anyways - I just noticed that there already exists a sdist_wheel job - perhaps it makes more sense to add a testing step to that job instead? WDYT @larsoner ?

@rossbar
Copy link
Contributor Author

rossbar commented Dec 2, 2025

I have to run to a meeting so I won't have a chance to look at this in the next hour - feel free to close/work elsewhere, otherwise I'll pick this up in the afternoon!

@larsoner
Copy link
Collaborator

larsoner commented Dec 2, 2025

Anyways - I just noticed that there already exists a sdist_wheel job - perhaps it makes more sense to add a testing step to that job instead? WDYT @larsoner ?

Sure!

This recipe behaves as expected (i.e. fails) locally, but something wonky is going on with the actions (not sure why they're not running).

In the actions tab you can see it's unhappy about some syntax

https://github.com/numpy/numpydoc/actions/runs/19871137814

(Line: 214, Col: 9): There's not enough info to determine what you meant. Add one of these properties: run, shell, uses, with, working-directory

rossbar and others added 5 commits December 3, 2025 12:38
* replace relpath in example_module tst fixture.

* Update __main__ rel to install path.

* Update importlib.resources.path incantation to
  a pattern that works on 3.12 and 3.14.

* Use cwd instead of requests.root_dir
@rossbar
Copy link
Contributor Author

rossbar commented Dec 3, 2025

Okay I think this is ready for review. It turned out to be more involved than I'd expected, sorry about the noise. The main issue was that some of the tests implicitly depended on being run from within the source tree. I tried to fix this with a minimal set of changes in 6298e1d, using importlib.resources to ensure that all paths to testing resources are derived from the packaged path instead of relative locations/locations in the source tree. If this solution passes the sanity check, then I'm sure there's more cleanup that can be done (e.g. getting rid of some fixtures/parametrizations, etc.). LMK if this looks okay to you @stefmolin

My original plan was to simply add the "build from sdist and test" workflow, but it turns out that even with the --pyargs flag, pytest will fall back to discovering files from the cwd, so that's not sufficient to guarantee that all necessary files are being pulled into the package. My workaround was to run the tests from somewhere other than the source tree in CI.

(tmp_path / file).touch()
else:
expected_dir = request.config.rootdir
expected_dir = Path.cwd()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this change is necessary to accommodate the running of tests from a different location, as there is then a difference between the root_dir reported by the request fixture and the location where the tests are being run.

@rossbar
Copy link
Contributor Author

rossbar commented Dec 3, 2025

A final thought: ultimately I think switching to cibuildwheel would significantly simplify both the testing and release process for wheels/source dists. That touches more infra though... this "hotfix" should at least be enough to provide temporary assurance that numpydoc isn't missing anything in the package.

@larsoner
Copy link
Collaborator

larsoner commented Dec 3, 2025

A final thought: ultimately I think switching to cibuildwheel would significantly simplify both the testing and release process for wheels/source dists

I thought cibuildwheel intentionally stayed away from providing sdist support (e.g., pypa/cibuildwheel#173), so I'm not clear on this part... I guess it would help us maybe at least test the .whl file by providing isolation etc.? Or maybe you mean we wouldn't need the sdist test if we had a wheel test that was executed separately from the project dir?

In any case things here are green so I think we're good to go!

@larsoner larsoner merged commit 98d8835 into numpy:main Dec 3, 2025
24 checks passed
@stefanv stefanv added this to the 1.10.0 milestone Dec 3, 2025
@rossbar
Copy link
Contributor Author

rossbar commented Dec 3, 2025

Or maybe you mean we wouldn't need the sdist test if we had a wheel test that was executed separately from the project dir?

Yeah this is what I was thinking; I'm suspecting (though not positive) that it would eliminate the need for this bespoke-out-of-source-tree CI recipe.

@rossbar rossbar deleted the test-sdist branch December 3, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants