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

Use pytest to run Python bindings tests #70

Merged
merged 6 commits into from
Feb 9, 2023
Merged

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Feb 8, 2023

Description

This PR solves the issue by installing pytest and running the tests with it. Low risk change here that simply enables easier tests for the future

Review guidelines

Estimated Time of Review: 2 minutes

@domire8 domire8 requested a review from eeberhard February 8, 2023 09:16
@domire8 domire8 linked an issue Feb 8, 2023 that may be closed by this pull request
Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

I imagine you won't be able to refactor all the tests in one go, so that there will be some unittest and some pytest cases. I think with dynamic test discovery you can probably run unittest and pytest back to back.

And, the build-test-python action still uses unittest, so when you start refactoring to use pytest you won't see the results in the CI. I think this would be a good place to also invoke pytest in that CI action. At the end of the test refactor you can then remove the unittest calls entirely.

Edit: as mentioned in the original issue #69, pytest will already discover and run unittest tests, so it's not necessary to invoke the test runner twice. Still, the CI should be updated.

python/Dockerfile.python Show resolved Hide resolved
@domire8
Copy link
Member Author

domire8 commented Feb 8, 2023

Edit: as mentioned in the original issue #69, pytest will already discover and run unittest tests, so it's not necessary to invoke the test runner twice. Still, the CI should be updated.

Thanks for thinking about the CI.
I see several possibilities:

  1. Installing pytest in the build-test-python action Dockerfile and wait until the next release where pytest will be installed in the base Docker image
  2. Dispatch the build-push workflow of the base Dockerfiles from this feature branch such that pytest is installed and then we don't need to install it in the action and the Python Dockerfile

@eeberhard
Copy link
Member

  1. Dispatch the build-push workflow of the base Dockerfiles from this feature branch such that pytest is installed and then we don't need to install it in the action and the Python Dockerfile

I think, given the very minor change to the base Dockerfile, I agree with manually building it from this branch. That way we don't have to remember to remove that line in a future step. If you do that, I would wait to close this PR until the image is built and the CI passes again.

@domire8 domire8 force-pushed the feature/use-pytest branch from adcae4e to 542d95e Compare February 8, 2023 17:02
@domire8 domire8 merged commit 31e68c0 into develop Feb 9, 2023
@domire8 domire8 deleted the feature/use-pytest branch February 9, 2023 06:29
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.

Move to pytest for Python bindings tests
2 participants