Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Aug 15, 2023

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.

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

if (is_legal_py_identifier(full_name) and
(notc_basenameoris_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
ContributorAuthor

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
ContributorAuthor

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

@erlend-aasland
Copy link
ContributorAuthor

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
ContributorAuthor

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

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

Looks great, thank you!

@erlend-aasland
Copy link
ContributorAuthor

thank you!

Likewise!

@erlend-aaslanderlend-aasland deleted the clinic/add-cloned-with-custom-c-basename-test branch August 15, 2023 20:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@erlend-aasland@AlexWaygood@bedevere-bot