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

Automatically register the schema tester pytest plugin #676

Merged
merged 10 commits into from
Apr 10, 2019

Conversation

drdavella
Copy link
Contributor

Previously packages were required to explicitly list the schema tester plugin in their conftest.py files. Now, the plugin is registered automatically with pytest.

The schema tests are no longer enabled by default. Instead, packages can do one of two things:

  1. Add asdf_schema_tests_enabled = true to the [tool:pytest] section of their setup.cfg file to enable tests by default, or
  2. Add --asdf-tests to the pytest command line to enable tests on a per-run basis.

The new plugin is in a different namespace than the previous plugin, so it will not cause conflicts with packages that still include the plugin in conftest.py. However, the old conftest.py configuration should be removed unless packages want to retain backwards compatibility with older versions of ASDF.

This closes #657.

cc @astrofrog

@drdavella drdavella added this to the v2.4.0 milestone Apr 9, 2019
@drdavella drdavella self-assigned this Apr 9, 2019
@stsci-bot
Copy link

stsci-bot bot commented Apr 9, 2019

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

1 similar comment
@stsci-bot
Copy link

stsci-bot bot commented Apr 9, 2019

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #676 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #676   +/-   ##
=======================================
  Coverage   93.34%   93.34%           
=======================================
  Files          39       39           
  Lines        4295     4295           
=======================================
  Hits         4009     4009           
  Misses        286      286

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8d8b3b...5ee090d. Read the comment docs.

@drdavella
Copy link
Contributor Author

Packaging the plugin in this way is proving to be unnecessarily burdensome. The right way to do it would be to move the plugin to another package entirely, but I don't think that's the right way to go.

@drdavella
Copy link
Contributor Author

Closing since this isn't really worth any more time/effort at the moment.

@drdavella drdavella closed this Apr 9, 2019
@astrofrog
Copy link
Contributor

Could you clarify what the issue is here with registering the pytest plugin?

@astrofrog
Copy link
Contributor

Packaging the plugin in this way is proving to be unnecessarily burdensome. The right way to do it would be to move the plugin to another package entirely, but I don't think that's the right way to go.

What would be wrong with moving the plugin to another package? In a way you already have a separate package for a sphinx plugin, so why not for a pytest plugin?

@drdavella
Copy link
Contributor Author

@astrofrog the issues have all been related to using the plugin to test the asdf package itself. I'm not sure I understand the root cause, but it seems to be related to the fact that the asdf package gets imported by pytest when the plugin is loaded, but also when test collection occurs. I think I found workarounds for most of these issues, but it's still causing failures on Windows for unknown reasons. I don't really want to abandon testing on Windows just because of this, but I'm not sure how much more time/effort I can afford at the moment. I'll try one more idea here now, but then I might have to leave this effort to someone else.

As for moving the plugin to another package, I think the future maintenance burden would be lower if it remained part of the same repository. However, with the way I have set things up here, the plugin is technically installed as a separate package.

@drdavella drdavella reopened this Apr 10, 2019
@drdavella drdavella force-pushed the pytest-plugin branch 2 times, most recently from 72468dd to 068839a Compare April 10, 2019 14:40
@drdavella
Copy link
Contributor Author

Looks like eliminating potentially circular references in the plugin got things working. Thanks @astrofrog for encouraging me to continue!

@drdavella drdavella merged commit cfcccb0 into asdf-format:master Apr 10, 2019
@drdavella drdavella deleted the pytest-plugin branch April 10, 2019 15:30
def resolver(self, uri):
tag_mapping = self.extension_list.tag_mapping
url_mapping = self.extension_list.url_mapping
return url_mapping(tag_mapping(uri))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. This is potentially useful elsewhere to get the full-bells-and-whistles resolver so that we don't have to instantiate a dummy AsdfFile object just to get to the resolver.

@jdavies-st
Copy link
Contributor

So is the following necessary in the top-level conftest.py for a package that would like to use asdf_schema_tester in order to work with both the old schema tester and new one via entry points?

import pkg_resources

entry_points = []
for entry_point in pkg_resources.iter_entry_points('pytest11'): 
    entry_points.append(entry_point.name)

if "asdf_schema_tester" not in entry_points:
    pytest_plugins = ['asdf.tests.schema_tester']

Plus enabling it in setup.cfg. Or is there a simpler way?

@drdavella
Copy link
Contributor Author

@jdavies-st since the new plugin is in a different namespace, I think there's actually no action required for packages that want to retain backwards compatibility. You will need to add asdf_schema_tests_enabled to your setup.cfg in order to enable the tests by default with the new plugin, but other than that there should be no issues.

@jdavies-st
Copy link
Contributor

But asdf.tests.schema_tester no longer exists in asdf namespace. So if we have

pytest_plugins = ['asdf.tests.schema_tester']

in our conftest.py, then we get a plugin import error. Our 2 test runs yesterday against asdf/master failed because of this:

https://travis-ci.org/spacetelescope/jwst/builds/518489243

https://travis-ci.org/spacetelescope/jwst/jobs/518489251#L685

@drdavella
Copy link
Contributor Author

Ah yeah good point. Yes I guess you do need logic like what you posted above, then. I wonder if it would be better to use the version of asdf as the condition though?

@jdavies-st
Copy link
Contributor

Agree. Once a version of asdf is released with the new plugin, then version would be better. Thanks for the suggestion. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install schema tester plugin using entry points
3 participants