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

Include test in sdist #2282

Merged
merged 2 commits into from Mar 17, 2023
Merged

Include test in sdist #2282

merged 2 commits into from Mar 17, 2023

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Mar 17, 2023

Summary of changes

Congratulations on 6.3.0!

A perhaps minor regression from earlier versions is that the sdist does not include the test folder, which makes it harder for downstreams to use a single source of truth to build and test a reliable package. This restores the test folder for sdists.

While the tests are of course large, most users' agents (modern pip) will prefer fetching the .whl, so this should create minimal disturbance for most users.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered adding an example in ./examples for new features.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@bollwyvl bollwyvl marked this pull request as ready for review March 17, 2023 12:43
Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for the PR, I will make 6.3.1 with this tonight still.

@aucampia
Copy link
Member

LGTM and thanks for the PR, I will make 6.3.1 with this tonight still.

That is, if kroki is going to play along 🤦

@coveralls
Copy link

Coverage Status

Coverage: 90.782%. Remained the same when pulling 86ad412 on bollwyvl:patch-1 into bea782f on RDFLib:main.

@aucampia aucampia merged commit e3884b7 into RDFLib:main Mar 17, 2023
23 checks passed
@aucampia
Copy link
Member

aucampia commented Mar 18, 2023

This is released now.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Mar 18, 2023 via email

@bollwyvl bollwyvl deleted the patch-1 branch March 18, 2023 15:54
@bollwyvl
Copy link
Contributor Author

Yep, the bot picked up the release just fine, but needs some help finding the new tests.

Of note: definednamespace_creator tests use a file from examples, but otherwise looking good!

@mgorny
Copy link

mgorny commented Mar 19, 2023

Of note: definednamespace_creator tests use a file from examples, but otherwise looking good!

Yeah, just wanted to report that it's missing. Could you add it too, please?

@aucampia
Copy link
Member

I will add examples to sdist, but I will also have a look at how you run the tests and see if we can't maybe run it similarly in our CI pipeline to ensure that it will work correctly for you.

@mgorny
Copy link

mgorny commented Mar 19, 2023

For the record, I'm from Gentoo and not conda-forge, so it's probably different but our code right now is kinda ugly: https://github.com/gentoo/gentoo/blob/master/dev-python/rdflib/rdflib-6.3.0.ebuild#L42

Long story short:

  • we strip doctest-modules option to disable tests using Internet
  • we strip some pytest-cov related option because we run tests without coverage testing
  • we explicitly deselect a bunch of tests (EPYTEST_DESELECT) and ignore one whole file (EPYTEST_IGNORE) that use Internet
  • we pass PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 because some plugin (it wasn't obvious which one and I haven't had time to check it more precisely) has caused the test suite to fail

That said, I haven't had time to recheck much of this, so perhaps there is a better way to solve some of these problems.

One thing that would really be helpful is having some marker on tests using Internet, so we could easily deselect them all.

@aucampia
Copy link
Member

aucampia commented Mar 19, 2023

One thing that would really be helpful is having some marker on tests using Internet, so we could easily deselect them all.

@mgorny I have made a new issue #2293 and will look at it when I have time.

@mgorny
Copy link

mgorny commented Mar 19, 2023

Thanks a lot!

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.

None yet

4 participants