pytest-dev / pytest-bdd

BDD library for the pytest runner
https://pytest-bdd.readthedocs.io/en/latest/
MIT License
1.32k stars 221 forks source link

Tag is missing for the 7.3.0 release #703

Closed ionenwks closed 2 months ago

ionenwks commented 2 months ago

Seen 7.3.0 was released on pypi but tag seems to be missing here (would use the pypi tarball, but it lacks tests). Thanks.

youtux commented 2 months ago

yep done, sorry. One day I will implement in CI this release process

youtux commented 2 months ago

why do you need tests in the distribution though?

webknjaz commented 2 months ago

(with my PyPA hat on)

@youtux the tests should be included in sdists but with the wheels, it depends on the repository structure. They are necessary for the downstreams to be able to rely on sdists when repackaging.

webknjaz commented 2 months ago

You should probably fill out the GitHub Releases entries for the tags, additionally to pushing to Git: https://github.com/pytest-dev/pytest-bdd/releases

webknjaz commented 2 months ago

One day I will implement in CI this release process

Here's my recent state of the art release automation example: https://github.com/ansible/awx-plugins/blob/e22c150/.github/workflows/ci-cd.yml#L748-L1150. And my PyPUG guide with a simpler example: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/. Hope this helps!

youtux commented 2 months ago

@youtux the tests should be included in sdists but with the wheels

is that documented/recommended anywhere by the PyPA or PyPI?

webknjaz commented 2 months ago

the tests should be included in sdists

To facilitate this, I even made an action that allows testing from sdist in upstream CIs: https://github.com/marketplace/actions/checkout-python-sdist

webknjaz commented 2 months ago

@youtux the tests should be included in sdists but with the wheels

is that documented/recommended anywhere by the PyPA or PyPI?

It's a recurring debate which boils down to whether individual upstream maintainers are willing to make downstream-friendly sdists. There's polarized opinions based on this distinction. The recent discussion I can recall was on dpo... I don't remember if we have that over on PyPUG, there were talks about documenting it on the issue tracker, though. Also, modern setuptools auto-include a number of discovered directories into sdists, especially when you start using PEP 621 metadata definition method. If they aren't, add them to MANIFEST.in. I usually rely on the setuptools-scm plugin which auto-includes everything tracked by Git...

The only thing to be careful about is that the top-level tests dirs should not end up in wheels, because they would end up in the root of site-packages. This is not such a problem for projects that have tests nested into the main installed package dir as they'll be scoped properly.

youtux commented 2 months ago

I think I found a way to include tests in the source dist, I made a package to try it out, could you confirm it's it satisfies your requirements? pytest_bdd-7.3.0.tar.gz

webknjaz commented 2 months ago

This is probably the best summary you can find on the topic: https://discuss.python.org/t/should-sdists-include-docs-and-tests/14578/117.

youtux commented 2 months ago

if that looks good to you, I'll merge https://github.com/pytest-dev/pytest-bdd/pull/704/

webknjaz commented 2 months ago

Can't properly check from phone but I posted some pointers over in the PR.

ionenwks commented 2 months ago

I think I found a way to include tests in the source dist, I made a package to try it out, could you confirm it's it satisfies your requirements? pytest_bdd-7.3.0.tar.gz

Running tests seem to be broken with this, albeit give me the impression this is just including commits it shouldn't.

ImportError: Error importing plugin "pytest_bdd.plugin": No module named 'gherkin'

Files difference between 7.3.0 tag and this 7.3.0 tarball, you'll likely know better what happened:

@@ -11,2 +2,0 @@
-./CHANGES.rst
-./CONTRIBUTING.md
@@ -13,0 +4 @@
+./PKG-INFO
@@ -15,5 +5,0 @@
-./docs
-./docs/Makefile
-./docs/conf.py
-./docs/index.rst
-./poetry.lock
@@ -21 +6,0 @@
-./pytest.ini
@@ -29,0 +15 @@
+./src/pytest_bdd/gherkin_parser.py
@@ -69 +55 @@
-./tests/feature/test_no_sctrict_gherkin.py
+./tests/feature/test_no_strict_gherkin.py
@@ -84,0 +71,5 @@
+./tests/parser
+./tests/parser/__init__.py
+./tests/parser/test.feature
+./tests/parser/test_errors.py
+./tests/parser/test_parser.py
@@ -97 +87,0 @@
-./tox.ini

gherkin aside, may also need to include pytest.ini, and it'd probably nice to have CHANGES.rst included. docs/ is handy if we want to build+install an offline copy of the docs.

Anyhow, tests do run fine for us with the 7.3.0 tag meanwhile :) And yes, we do want to ensure things are not broken w/ current dependencies when we repackage it downstream, this is further relevant when we add support for a new python version as we wouldn't let it use a new one if the tests don't pass yet (unless can determine it's just the test itself that needs updates rather than the plugin being broken anyway).

Edit: we can keep using the tag if needed, albeit we do prefer official sdists over automatically generated github archives when possible

musicinmybrain commented 2 months ago

As the maintainer of the python-pytest-bdd package in Fedora, I appreciate it when PyPI sdists are “complete,” but I end up using GitHub archives instead for approximately half of the Python packags I work on, because there is so often something missing that I want for the distribution package – tests, ancillary files for testing like a test-requirements.txt or tox.ini file, or documentation that I actually want to ship, like a changelog. Our guidelines allude to this – “Packages MAY use sources from PyPI. However, packages SHOULD NOT use an archive that omits test suites, licenses and/or documentation present in other source archives.”

Generally, I don’t even try to convince upstreams to put more in their sdists, because every project seems to have a different philosophy on what should go in an sdist (if they’ve even looked closely at their sdists), and few consider making sdists suitable for distribution packaging to be a useful goal. In the end, as long as the tags are pushed and I can package from the automatic GitHub archives, I’m happy.

Just another perspective.

youtux commented 2 months ago

thank you for the explanation. I find it becoming too cumbersome maintaining a build that creates an sdist which is basically a checkout of the project.

I will stick to releasing the sdist with the bare minimum contents.

youtux commented 2 months ago

at least now I moved the release to PyPI to a GH actions that is triggered when pushing tags, so we can be sure that releases are always tagged now.

webknjaz commented 2 months ago

@youtux basically, this is because of such complexity I made the action to make sure sdists are always usable and setuptools-scm auto-including everything everything Git-tracked completes the setup.

Technically, sdists should be complete project checkouts as it's their only function. Distributions are never installed directly, not anymore. When pip encounters a project only publishing an sdist, it first makes a wheel out of it and only then installs it (by unzipping that wheel).

It's good that you added python -m build without the args since it emulated what pip does. However, you might still be testing the project "as checked out from Git" instead of testing "as installed" which might yield different behavior in some corner cases.

This is the second reason I made that action — to make sure there's nothing on disk that influences what's tested by accident.

Here's some other approaches: https://blog.ganssle.io/articles/2019/08/test-as-installed.html.

youtux commented 2 months ago

yeah if that was encouraged by PyPA and maintained by PyPA I'd gladly do that as I'd find it a stable solution.

youtux commented 1 month ago

Kind of related to this, I made a PR to test against the artifact that we will upload to PyPI, @webknjaz want to maybe have a look?

webknjaz commented 1 month ago

Thanks, I left an initial comment. Only one thing stands out so far.