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-106368: Argument Clinic: Add tests for cloned functions with custom C base names #107977

Conversation

erlend-aasland
Copy link
Contributor

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

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.

There is still one case in this block that is untested:

cpython/Tools/clinic/clinic.py

Lines 4888 to 4890 in e28b0dc

if (is_legal_py_identifier(full_name) and
(not c_basename or is_legal_c_identifier(c_basename)) and
is_legal_py_identifier(existing)):

What if is_legal_py_identifier(full_name) evaluates to True, not c_basename evaluates to False, is_legal_c_identifier(c_basename) evaluates to False and is_legal_py_identifier(existing) evaluates to True?

I.e., if I apply this diff to your PR branch, the assertion is never triggered:

-            if (is_legal_py_identifier(full_name) and
-                (not c_basename or is_legal_c_identifier(c_basename)) and
-                is_legal_py_identifier(existing)):
+            if is_legal_py_identifier(full_name) and is_legal_py_identifier(existing):
+                if c_basename:
+                    assert is_legal_c_identifier(c_basename)

...Could you also add a test with a cloned function with a custom C base name where the custom C base name is not a legal C identifier?

@AlexWaygood
Copy link
Member

I'm getting warnings about the execution environment when I'm running python -m test test_clinic -v with this PR branch locally, btw:

Ran 243 tests in 0.620s

OK
Warning -- files was modified by test_clinic
Warning --   Before: []
Warning --   After:  ['clinic/']
Warning -- files was modified by test_clinic
Warning --   Before: []
Warning --   After:  ['clinic/']
test_clinic failed (env changed)

== Tests result: SUCCESS ==

1 test altered the execution environment:
    test_clinic

Total duration: 1.2 sec
Tests result: SUCCESS

@AlexWaygood
Copy link
Member

I'm getting warnings about the execution environment when I'm running python -m test test_clinic -v with this PR branch locally, btw:

Ah, and it looks like the CI is complaining about the same thing: https://github.com/python/cpython/actions/runs/5868181574/job/15910445072

@erlend-aasland
Copy link
Contributor Author

I'm getting warnings about the execution environment when I'm running python -m test test_clinic -v with this PR branch locally, btw:

Ah, and it looks like the CI is complaining about the same thing: https://github.com/python/cpython/actions/runs/5868181574/job/15910445072

Ah, yeah, I noticed earlier today, but got sidetracked. Since we're running a "expect success" test, we will actually generate clinic output in the added test (hence the sudden clinic/ directory). A workaround is to register a cleanUp() method, another is to just suppress all output, and yet another, is to run a destination file clear at the end of the clinic input.

@erlend-aasland
Copy link
Contributor Author

I wonder why we're not seeing complaints about clinic/ directories for the other self.clinic.parse tests...

@erlend-aasland
Copy link
Contributor Author

I wonder why we're not seeing complaints about clinic/ directories for the other self.clinic.parse tests...

... because they don't create output; either they're dumping to block or buffer, or they're testing some weird directive, or they're run with precomputed checksums in the input (hence no regenerated output).

@erlend-aasland
Copy link
Contributor Author

...Could you also add a test with a cloned function with a custom C base name where the custom C base name is not a legal C identifier?

Ah, good call. On it!

@AlexWaygood AlexWaygood changed the title gh-106368: Argument Clinic: Add test for cloned function with custom C base name gh-106368: Argument Clinic: Add tests for cloned functions with custom C base names Aug 15, 2023
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.

Looks great, thank you!

@erlend-aasland
Copy link
Contributor Author

thank you!

Likewise!

@erlend-aasland erlend-aasland enabled auto-merge (squash) August 15, 2023 20:41
@erlend-aasland erlend-aasland merged commit bb456a0 into python:main Aug 15, 2023
17 checks passed
@erlend-aasland erlend-aasland deleted the clinic/add-cloned-with-custom-c-basename-test branch August 15, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants