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

Add regression test for generated package data #432

Merged
merged 12 commits into from
Nov 1, 2024
Merged

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Jul 24, 2024

This adds a regression test for generated pacakge data. The test folder now has a data folder inside it which contains a reference copy of the generated package. This is then compared at test time to a newly generated package. Any differences are shown after the failing test with a diff that looks like:

E           RuntimeError: Non-zero diff between generated files and expected files.
E           Generated files can be found in /Users/dstansby/software/python-tooling/pytest_output/test_package_generation0/cookiecutter-test.
E
E           --- /Users/dstansby/software/python-tooling/pytest_output/test_package_generation0/cookiecutter-test/README.md
E           +++ /Users/dstansby/software/python-tooling/tests/data/test_package_generation/README.md
E           @@ -119,7 +119,7 @@
E            tox -e docs
E            ```
E            
E           -from the root of the repository. The built docs will be written to
E           +from the root of the repository. The built documentation will be written to
E            `site`.
E            
E            Alternatively to build and preview the documentation locally, in a Python

tests/test_package_generation.py:79: RuntimeError

To update the test files with an intentional change, when the test fails the test data is updated with the changes that can then be easily comitted (if desired).

Fixes #329

@dstansby dstansby marked this pull request as draft July 24, 2024 15:04
@dstansby dstansby force-pushed the regression-tests branch 3 times, most recently from 712325c to ccfed73 Compare July 24, 2024 15:48
@dstansby dstansby changed the base branch from main to robust-git-test July 24, 2024 15:48
Base automatically changed from robust-git-test to main August 8, 2024 14:15
@dstansby dstansby force-pushed the regression-tests branch 3 times, most recently from a285ab3 to db634fb Compare October 22, 2024 10:41
pyproject.toml Outdated
"tests",
[tool.mypy]
exclude = [
'tests\/data\/test_package_generation\/src\/cookiecutter_test\/__init__\.py',
Copy link
Member Author

Choose a reason for hiding this comment

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

Can someone work out how I can get this to work? mypy doesn't seem to be ignoring the file 😢

Copy link
Member

Choose a reason for hiding this comment

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

So I encountered this problem yesterday (along with @alessandrofelder) python/mypy#17977. It's not ideal, but maybe we just exclude from pre-commit?

@dstansby dstansby marked this pull request as ready for review October 22, 2024 10:45
@dstansby dstansby requested a review from a team October 22, 2024 10:45
@paddyroddy paddyroddy self-requested a review October 22, 2024 11:03
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Are we sure we want this? Every time there's a new change, this test will have to be updated.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@dstansby
Copy link
Member Author

Are we sure we want this? Every time there's a new change, this test will have to be updated.

See #329 for context and motivation.

Definitely appreciate that this adds a bit more work when changing anything in the template, but I think it's worth it to catch potential errors such as in #309 which slipped through the PR author and reviewer.

I've tried to make the loop here as simple as possible:

  1. Run tests
  2. If this test fails, it modifies the test data in place
  3. Review if the test data is modified as expected. If it is, commit and push the changes.

@paddyroddy
Copy link
Member

paddyroddy commented Oct 22, 2024

See #329 for context and motivation.

Definitely appreciate that this adds a bit more work when changing anything in the template, but I think it's worth it to catch potential errors such as in #309 which slipped through the PR author and reviewer.

Consider me won over, I'd forgotten about #309

@paddyroddy paddyroddy added the needs-2-reviewers Considered "controversial" so worth a second pair of eyes label Oct 22, 2024
@dstansby
Copy link
Member Author

Sigh, putting this back as draft until I can fix the test

@dstansby dstansby marked this pull request as draft October 22, 2024 18:50
@paddyroddy paddyroddy mentioned this pull request Oct 23, 2024
1 task
@dstansby dstansby force-pushed the regression-tests branch 2 times, most recently from 698df36 to 882c939 Compare October 24, 2024 08:48
@dstansby dstansby marked this pull request as ready for review October 24, 2024 08:55
@dstansby dstansby requested review from paddyroddy and a team October 24, 2024 08:55
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Can you fix (by which I mean exclude) the link checking failures?
image

Fix test data

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fix toml sorting
@paddyroddy
Copy link
Member

The tests are failing again @dstansby. main had updated.

Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

I've fixed this @dstansby. Will approve and merge now so it doesn't need fixing again.

@paddyroddy paddyroddy merged commit 30efb51 into main Nov 1, 2024
12 checks passed
@paddyroddy paddyroddy deleted the regression-tests branch November 1, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-2-reviewers Considered "controversial" so worth a second pair of eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some regression tests on generated package files
2 participants