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

Improve testsuite #1975

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

hulkoba
Copy link

@hulkoba hulkoba commented Jul 12, 2024

Hello 👋!

As part of the STF work for Conda, this PR introduces several improvements to our testing framework by transitioning from unittest to pytest, leveraging pytest-mock for mocking, and adopting snapshot testing with syrupy.

Note that the testing section in the readme is updated at #1956

Thank you!

Checklist

  • Added a news entry

@hulkoba hulkoba requested a review from a team as a code owner July 12, 2024 08:26
@hulkoba
Copy link
Author

hulkoba commented Jul 12, 2024

Somehow ignoring the snapshots from linting doesn't work in this branch as it does for the one on the fork neighbourhoodie#13

@hulkoba hulkoba changed the title Modernize testsuite Improve testsuite Jul 12, 2024
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.

So the changes to the tests as written are wrong in many cases.

Many of the tests verify the correctness of the results as opposed to being regression tests handled by snapshotting. For tests that verify result correctness, we hard code the correct answer clearly in the test case. In some cases we even include comments on why the answer is correct. The changes here obfuscate that.

We need to separate these two cases out and only apply snapshotting to true regression tests that only care if the code output did not change and have no constraints on what is in that output.

@jcoglan
Copy link

jcoglan commented Jul 15, 2024

@beckermr Hi 👋 I work with @hulkoba at @neighbourhoodie and I'm following up while she's on vacation.

Your comment about snapshot testing absolutely makes sense. Do you have a list of which tests you think we should use snapshots for, and which should remain as hard-coding the expected response?

@beckermr
Copy link
Member

I'd say all of them should remain hard coded.

@hulkoba
Copy link
Author

hulkoba commented Aug 28, 2024

Hey all 👋

Since the snapshot testing is still discussed, I was wondering if you are still interested in the removal of unittest in favor of pytest?

If so, I could take those commits from this PR and create a new one.
What do you think?

@beckermr
Copy link
Member

Yes, for sure we should move to pytest!

@hulkoba hulkoba mentioned this pull request Aug 29, 2024
1 task
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.

4 participants