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.

Sign up for GitHub

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

Jump to bottom

Run pytest tests #297

Merged
merged 6 commits into from
Jul 19, 2024
Merged

Run pytest tests #297

merged 6 commits into from
Jul 19, 2024

Conversation

baszalmstra
Copy link
Member

@baszalmstra baszalmstra commented Jul 19, 2024 •
edited
Loading

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

This PR adds running of the pytest to the recipe test section. This is useful so that we can run downstream tests from dependencies to guard against accidental breakage.

I also added pydantic >=2,<3 to the run requirements. This is also the case in the upstream environment.yaml.

@baszalmstra baszalmstra requested a review from a team as a code owner July 19, 2024 08:42
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@baszalmstra
Copy link
Member Author

Ouch that is a lot of errors..

@baszalmstra
Copy link
Member Author

I only included specific tests for now. I am unsure why a lot of tests fail but I assume its because the tests do not expect to be run from another repository?

Anyway, I think its still important to be able to run the majority of the tests for downstream tests.

@baszalmstra
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions GitHub Actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-smithy-feedstock/actions/runs/10006117311.

@beckermr
Copy link
Member

Pydantic is not a runtime requirement afaik. Cc @isuruf who knows this better.

beckermr
beckermr requested changes Jul 19, 2024
View reviewed changes
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

There is a specific pytest invocation to use for installed packages. We should try that here.

@baszalmstra
Copy link
Member Author

There is a specific pytest invocation to use for installed packages. We should try that here.

Would be happy to! Do you know what the invocation is, a quick google didnt tell me much?

@baszalmstra
Copy link
Member Author

Pydantic is not a runtime requirement afaik. Cc @isuruf who knows this better.

Its used here:

https://github.com/conda-forge/conda-smithy/blob/7b0b83038ae6c012c72bc13cca6641fb81dc7e08/conda_smithy/schema.py#L10

I think thats part of the package.

@beckermr
Copy link
Member

We currently serialize the schema as json and don't use pydantic at runtime IIRC.

@baszalmstra
Copy link
Member Author

We currently serialize the schema as json and don't use pydantic at runtime IIRC.

But then import conda_smithy.schema would just fail right?

@beckermr
Copy link
Member

True but I don't think that module is ever impoted at runtime when using smithy.

@beckermr
Copy link
Member

See here:

https://github.com/search?q=repo%3Aconda-forge%2Fconda-smithy%20conda_smithy.schema&type=code

this is by design AFAIK. Other members of @conda-forge/core should confirm or deny.

beckermr
beckermr reviewed Jul 19, 2024
View reviewed changes
recipe/meta.yaml Outdated
Comment on lines 81 to 82
- pytest tests/test_condaforge_config_schema.py tests/test_ci_skeleton.py tests/test_feedstock_io.py tests/test_feedstock_tokens.py tests/test_lint_recipe.py tests/test_variant_algebra.py

Copy link
Member

Choose a reason for hiding this comment

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

Let's use --ignore here so that we don't have to update this list.

beckermr
beckermr requested changes Jul 19, 2024
View reviewed changes
Copy link
Member

@beckermr beckermr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See the comment above.

@baszalmstra
Copy link
Member Author

See here:

github.com/search?q=repo:conda-forge/conda-smithy%20conda_smithy.schema&type=code

this is by design AFAIK. Other members of @conda-forge/core should confirm or deny.

I wonder why that would be by design because that seems like bad practice. I can imagine it is used just to generate the JSON schema but I dont think it belongs inside the package in that case.

But I would happy to remove it, Ill wait for someone from core to reply.

@beckermr
Copy link
Member

I do not want to rehash the pydantic issue since it has been an ongoing thing. I doubt this PR will get merged until it is removed.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@baszalmstra
Copy link
Member Author

I do not want to rehash the pydantic issue since it has been an ongoing thing. I doubt this PR will get merged until it is removed.

Ok! I moved it to the test section instead.

beckermr
beckermr reviewed Jul 19, 2024
View reviewed changes
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

We don't need a new build.

recipe/meta.yaml Outdated Show resolved Hide resolved
@beckermr beckermr added the automerge Merge the PR when CI passes label Jul 19, 2024
@github-actions github-actions bot merged commit 26f99d0 into conda-forge:main Jul 19, 2024
4 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beckermr beckermr beckermr approved these changes

Assignees
No one assigned
Labels
automerge Merge the PR when CI passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@baszalmstra @beckermr