Skip to content

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.

@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.

@dstansby dstansby requested a review from paddyroddy October 22, 2024 11:55
@paddyroddy paddyroddy removed their request for review October 22, 2024 13:06
@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 Could be 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 a team and paddyroddy 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 Could be 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