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

gh-95065: Argument Clinic: Add functional tests of deprecated positionals #107768

Merged
merged 16 commits into from
Aug 10, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 8, 2023

Move the "deprecated positinal" tests from clinic.test.c to
_testclinic.c. Mock PY_VERSION_HEX in order to prevent the generated
compiler warnings/errors to trigger. Put clinic code for deprecated
positionals in Modules/clinic/_testclinic_depr_star.c.h for easy
inspection of the generated code.

…ositionals

Move the "deprecated positinal" tests from clinic.test.c to
_testclinic.c. Mock PY_VERSION_HEX in order to prevent the generated
compiler warnings/errors to trigger. Put clinic code for deprecated
positionals in Modules/clinic/_testclinic_depr_star.c.h for easy
inspection of the generated code.
@erlend-aasland
Copy link
Contributor Author

The big diff is caused by moving the generated clinic code.

@erlend-aasland
Copy link
Contributor Author

@serhiy-storchaka, IMO this is better than visual inspection of the generated code. We want to prevent regressions because of future refactorings, features and bugfixes.

@AlexWaygood
Copy link
Member

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Build FAILED.

C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(11,1): error C2133: 'deprecate_positional_pos0_len1__doc__': unknown size [C:\Users\
alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(98,1): error C2133: 'deprecate_positional_pos0_len2__doc__': unknown size [C:\Users\
alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(188,1): error C2133: 'deprecate_positional_pos0_len3_with_kwd__doc__': unknown size
[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(287,1): error C2133: 'deprecate_positional_pos1_len1_optional__doc__': unknown size
[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(383,1): error C2133: 'deprecate_positional_pos1_len1__doc__': unknown size [C:\Users
\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(473,1): error C2133: 'deprecate_positional_pos1_len2_with_kwd__doc__': unknown size
[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(572,1): error C2133: 'deprecate_positional_pos2_len1__doc__': unknown size [C:\Users
\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(664,1): error C2133: 'deprecate_positional_pos2_len2__doc__': unknown size [C:\Users
\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(758,1): error C2133: 'deprecate_positional_pos2_len3_with_kwd__doc__': unknown size
[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]
    0 Warning(s)
    9 Error(s)

Lib/test/test_clinic.py Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Does it still happen?

output push
destination deprstar new file '{dirname}/clinic/_testclinic_depr_star.c.h'
output everything deprstar
#output methoddef_ifndef buffer 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output directive has a bug that prevents us from specifying the stack index. We should fix that in a separate PR.

@AlexWaygood
Copy link
Member

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Does it still happen?

Nope! Builds fine locally now :)

Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Would not be better to use version like 100.200 to avoid rewriting tests every year?

Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Would not be better to use version like 100.200 to avoid rewriting tests every year?

No need, we mock the version.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Do the test fail if change the stacklevel argument in PyErr_WarnEx() from 1 to 2?

@erlend-aasland
Copy link
Contributor Author

Do the test fail if change the stacklevel argument in PyErr_WarnEx() from 1 to 2?

They do not. The same filename is produced for both stack levels.

@serhiy-storchaka
Copy link
Member

And wrong stacklevel is a common error. Even your original code did have it wrong initially. Therefore, I think it is important to check it using one of the methods I suggested above.

@erlend-aasland
Copy link
Contributor Author

Note: we will get merge conflicts from #107808.

Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I agree with Serhiy that explicitly testing the stacklevel would be good. Other than that, this is looking much better now, thanks! The readabiliy is much improved :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I'm happy if Serhiy's happy!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Few nitpicks and LGTM.

Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Modules/_testclinic.c Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Show resolved Hide resolved
@erlend-aasland erlend-aasland enabled auto-merge (squash) August 10, 2023 06:54
@erlend-aasland
Copy link
Contributor Author

Thank you Serhiy and Alex; this PR is in much better shape now! With this, we've got a nice framework for similar tests in place. I'll start working on what's left in clinic.test.c.

@erlend-aasland erlend-aasland merged commit 39ef93e into python:main Aug 10, 2023
15 checks passed
@erlend-aasland erlend-aasland deleted the clinic/better-depr-tests branch August 10, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants