Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-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
ContributorAuthor

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

@erlend-aasland
Copy link
ContributorAuthor

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

@erlend-aasland
Copy link
ContributorAuthor

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
ContributorAuthor

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 :)

Copy link
Member

@serhiy-storchakaserhiy-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?

@erlend-aasland
Copy link
ContributorAuthor

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-storchakaserhiy-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
ContributorAuthor

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
ContributorAuthor

Note: we will get merge conflicts from #107808.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

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

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

@erlend-aaslanderlend-aasland enabled auto-merge (squash) August 10, 2023 06:54
@erlend-aasland
Copy link
ContributorAuthor

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-aaslanderlend-aasland merged commit 39ef93e into python:mainAug 10, 2023
@erlend-aaslanderlend-aasland deleted the clinic/better-depr-tests branch August 10, 2023 07:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@AlexWaygood@serhiy-storchaka@bedevere-bot