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

Migrate to pytest, add Github action for test suite #186

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mathiasertl
Copy link
Collaborator

  • Move the test suite to pytest.
  • Add GitHub workflow for the test suite.
  • Add a Docker image that can be used for testing.
  • Test on Python 3.13.
  • Move dev-dependencies to uv.

@mathiasertl
Copy link
Collaborator Author

@danni @kislyuk I would appreciate a reviews.

@mathiasertl
Copy link
Collaborator Author

PS: Branch that adds strict typehints is ready after this one ;-).

@kislyuk
Copy link
Member

kislyuk commented Dec 15, 2024

Thanks, I'm working on this in the next couple of days.

@kislyuk
Copy link
Member

kislyuk commented Dec 15, 2024

@mathiasertl this PR is very difficult to review because it's massive. Can you please split it into fewer more manageable PRs?

@kislyuk
Copy link
Member

kislyuk commented Dec 16, 2024

@mathiasertl let's hold off on this PR. It should be done much more incrementally, and I think some of them (such as the use of pytest and Docker) need to be much more deliberately justified as they don't seem relevant to the core functions of the package itself. I am also working on some changes that will conflict with these.

@mathiasertl
Copy link
Collaborator Author

Hi,

Fair point, I'll move some of the unrelated stuff to a separate PR.

Docker: it's just an additional file to run the test suite, it's not used anywhere else. I wrote it because for unknown reasons, softhsm access in GitHub doesn't work with the package from APT (that's why it's compiled just like it was in TravisCI, see notes there). Since I already had it, I thought I might commit it just as well. If you want, I can remove it of course?

Pytest: it's what everyone uses, for good reason. It's almost impossible to do this incrementally without code duplication. If you ignore whitespace, you'll see that the test code itself has not changed at all. If you insist, I can make incremental PRs for that?

Kr, Mat

@kislyuk
Copy link
Member

kislyuk commented Dec 16, 2024

OK, thanks for elaborating - let's keep the Dockerfile in a separate PR for now until I can understand the issue better.

I am not completely opposed to Pytest but I find the argument that "everyone uses it and therefore we must" extremely tiresome. I think there should be a high bar to switching away from solutions provided by the standard library (unittest) to external ones. I like pytest but I think its fixture autoloading process is poorly designed, and I also like unittest and its inheritance/class based test case organization just as well and I don't see a specific reason to switch this project to pytest so far. If such reasons are identified, then at minimum, we should take the incremental approach of first switching to pytest and using its unittest compatibility layer, and only then switching tests to the pytest style where needed.

@kislyuk
Copy link
Member

kislyuk commented Dec 16, 2024

I just looked more carefully at the PR and yeah, I'm not on board with using conftest.py and the fixtures in it. That's the poorly designed part of pytest that I was talking about. Let's keep this type of setup in unittest setup methods for legibility.

@mathiasertl mathiasertl force-pushed the feature/github-actions-for-tests branch from 122e05d to 856b3a8 Compare December 19, 2024 17:18
@mathiasertl mathiasertl force-pushed the feature/github-actions-for-tests branch from 856b3a8 to d17d4af Compare December 27, 2024 12:52
@mathiasertl mathiasertl force-pushed the feature/github-actions-for-tests branch from 24cb12b to f3979bc Compare December 27, 2024 14:14
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.

2 participants