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

pygettext: Clean up obsolete tests and improve test coverage #130197

Open
StanFromIreland opened this issue Feb 16, 2025 · 6 comments
Open

pygettext: Clean up obsolete tests and improve test coverage #130197

StanFromIreland opened this issue Feb 16, 2025 · 6 comments
Assignees
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@StanFromIreland
Copy link
Contributor

StanFromIreland commented Feb 16, 2025

ast.get_docstring is now used making many tests obsolete.

Linked PRs

@picnixz picnixz added the tests Tests in the Lib/test dir label Feb 16, 2025
@picnixz
Copy link
Member

picnixz commented Feb 16, 2025

As I said on the PR, even if the tests may be redundant, they are still checking that running the tool with --docstrings option works well. So they are more than just one-shot tests, they can also be thought as E2E tests.

@tomasr8
Copy link
Member

tomasr8 commented Feb 16, 2025

As I said on the PR, even if the tests may be redundant, they are still checking that running the tool with --docstrings option works well. So they are more than just one-shot tests, they can also be thought as E2E tests.

A lot of them are redundant, for instance test_funcdocstring_bytes just tests that ast.get_docstring works correctly, or test_funcdocstring_annotated_return which was needed for the old tokenize code (and there are more such tests). A lot of the options are also already covered in the snapshot tests as well.

In addition to removing some of the tests, we might also want to add one test that checks that the docstrings are not extracted when the --docstring option is not passed.

@picnixz
Copy link
Member

picnixz commented Feb 16, 2025

Ok for removing the tests, but it would be good that we have an E2E test for --docstring

@tomasr8
Copy link
Member

tomasr8 commented Feb 16, 2025

Ok for removing the tests, but it would be good that we have an E2E test for --docstring

Yes of course, that is not going away, I actually think the we need more tests for the CLI in general, but the other tests just get in the way for little gain

@picnixz picnixz added the type-feature A feature request or enhancement label Feb 17, 2025
@serhiy-storchaka
Copy link
Member

The use of ast.get_docstring is an implementation detail. The tests should stay to ensure that no regression will be introduced when we change the implementation next time.

@tomasr8
Copy link
Member

tomasr8 commented Feb 18, 2025

I see, I didn't consider the (hopefully unlikely!) possibility of changing the implementation. In that case, let's keep the tests and let's focus on actually fully testing the CLI - there are still many options/flags which are not tested at all currently.

@picnixz picnixz added the pending The issue will be closed if no feedback is provided label Feb 24, 2025
@tomasr8 tomasr8 changed the title pygettext: Clean up obsolete tests pygettext: Clean up obsolete tests and improve test coverage Feb 24, 2025
@tomasr8 tomasr8 removed the pending The issue will be closed if no feedback is provided label Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement
Projects
Status: No status
Development

No branches or pull requests

4 participants