Make pybind11 test fixture fully self-contained

[bluenote10]

Have you read the Contributing Guidelines?

Yes.

Description

This PR improves the testing concept for pybind11 based stub generator.

Currently the stubgen test installs an external project called pybind11-mypy-demo from PyPI. This makes the test approach non-self-contained and a little bit confusing: When looking at the test fixture in the test-data folder, the obvious question is: Where is the corresponding C++ pybind11 code? Only after looking up the scripts in misc/test-stubgenc.sh one can guess that this actually is an external dependency. Then one has to lookup the code in the external repository and make the connection to the content of the expected stubgen output

As a result, contributing new pybind11 test cases is awkward. In case one wants to add a test for a new pybind11 pattern / expression / edge case, one first has to create a pull request for the external repository. This PR then has to be merged and published on PyPI. Only once the package has been deployed to PyPI the corresponding PR for mypy can be opened. This 3-step approach is tedious enough that contributors will tend not to add new test cases at all. For instance PR #12524 mentions new pybind11 related test patterns in its description, but these test cases were not contributed, most likely due to the overhead. I think it would help to follow testing best practices by trying to keep things self-contained.

This PR now moves the "source of truth" of the stubgen test fixtures into this repository. The idea is to keep the pybind11 example source close to the expected stubgen output, so that it becomes obvious that the two are connected, and when making a change, the necessary changes are close to each other. The "project" is basically just one main.cpp with minor boilerplate for the package setup. So far I've copied everything over here without any modifications, except the dummy test, which isn't needed here.

@sizmailov I hope you are fine with this change? I've just copied over the license from your project as well, but it doesn't mention your name I'm afraid. Let me know if I can do anything to improve the attribution. Taking this as an opportunity to thank you for your much appreciated work on pybind11 + mypy in general ;)

Test Plan

See above, because this PR is all about a change to the test plan.

[bluenote10]

The failed test highlights another issue with the current approach: In fact, the currently included reference stubgen output is already outdated, and cannot be reproduced with recent pybind11 versions. The reason this wasn't noticed so far is that the pybind11 version currently is "baked" into the wheels on PyPI. I.e., it only works because the wheels have been generated using some old pybind11 version, and it is currently even unclear what pybind11 version one would have to use to reproduce the reference stubgen output (I assume pre 2.9). This can be verified by using

python -m pip install pybind11_mypy_demo==0.0.1 --no-binary :all:

in the test-stubgenc.sh, which bypasses the wheels and installs the current version of the demo project from source, internally pulling the latest pybind11 version. This also creates the diff that I had to apply to the stubgen reference output now.

In general, we have two options:

• leave the pybind11 version unpinned, and testing against the latest version. This has the issue that CI could break from one day to the other when pybind11 updates. An upper bound on the pinning could mitigate that, e.g. pybind11<3.
• fully pin the pybind11 version. In this case everything is fully deterministic, but the pybind version would have to be bumped manually from time to time.

What would you prefer? I've implemented the second solution currently, because on master the pybind11 version is kind of "freezed" as a result of the wheel installation as well. We could also go for a parametrized/matrix CI job that tests against multiple versions, but I could imagine that the pybind11 version isn't important enough to justify the complexity.

For now I have solved the pinning of the pybind11 version inside the demo project pyproject.toml, which is a little bit awkward. An alternative would be to pin it inside the test-requirements.txt and then install the demo project with --no-build-isolation i.e., use the pybind11 version from the outer venv instead of relying of a pybind11 version being pull within the bootstrapping venv.

[sizmailov]

Hi!

I'm Ok with the change, but I think it's not my call.

Initially, I was inclined to do it similarly to this PR, but I was advised otherwise. I don't know if anything behind that decision changed. Please refer to #9877.

Pinging @JelleZijlstra @JukkaL due to their involvement in #9877

[bluenote10]

@sizmailov Cool, thanks for the feedback!

@JukkaL For all the reasons outlined above, I really don't think separating out the C++ code in an external PyPI package is a good strategy. The fact that the previous PRs missed to contribute valuable new test cases should emphasize that this strategy is suboptimal. Looking at #12524 again, it has fixed something about the stubgen, but the functionality it contributes is not even tested currently: The changes to the stubgen file were unrelated. In order to fix its actual feature it would have been necessary to add a new struct to the C++ code. The reason I'm contributing this PR is that I actually wanted to fix #12717 which was caused by #12524. But before fixing it, I wanted to add a new test case that ensures that my fix will not just break the feature introduced in #12524. As far as I can see, currently I could just revert it without breaking anything because it has no test case.

If the overhead of having a pyproject.toml + setup.py + LICENSE is deemed too much, we could simplify it down to:

• just keep the main.cpp. It is the only thing that is needed really. We could move the license as an inline license.
• just run something like the following in the test-stubgenc.sh:

  c++ -O3 -Wall -shared -fPIC $(python3 -m pybind11 --includes) main.cpp -o <some-output-name>.so

[JukkaL]

I can see how it will make CI more effective if we build the demo project during each CI run. The approach seems reasonable.

Would somebody who has context on pybind11 be able to review the changes (even if you aren't a regular contributor)? I can merge this PR once it has been reviewed.

[bluenote10]

I have now minimized the pybind_mypy_demo to the bare minimum. This makes more sense in the mypy test context here compared to its original purpose of e.g. documenting possible alternatives in the setup.py in a "demo" sense.

[bluenote10]

Would somebody who has context on pybind11 be able to review the changes (even if you aren't a regular contributor)? I can merge this PR once it has been reviewed.

I'm not sure if any pybind11 experts except for sizmailov are tracking this issue. But there is also little to review on C++ side, because the example is just the existing demo code anyway.

The more interesting part I guess is the CI pipeline integration. Do I need to ping someone who feels responsible for that?

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[bluenote10]

@JukkaL Could we merge this so that we have the basis for follow up PRs related to the stubgen?