GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-94808: Cover `PyOS_mystrnicmp` and `PyOS_mystricmp` by artemmukhin · Pull Request #102469 · python/cpython

Conversation

@artemmukhin

@artemmukhin artemmukhin commented Mar 6, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Metabug: Improving C-level coverage #94808

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@artemmukhin
pythonGH-94808: Cover PyOS_mystrnicmp and PyOS_mystricmp
21c6519
@bedevere-bot

bedevere-bot commented Mar 6, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot mentioned this pull request Mar 6, 2023
Metabug: Improving C-level coverage #94808
Open
@bedevere-bot bedevere-bot added the awaiting review label Mar 6, 2023
@ghost

ghost commented Mar 6, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@artemmukhin

artemmukhin commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

I'm not sure who is best positioned to review this, but maybe @corona10 could take a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 self-requested a review March 10, 2023 15:05
@corona10 corona10 added the skip news label Mar 10, 2023
@corona10

corona10 commented Mar 10, 2023

Copy link
Copy Markdown
Member

I'm not sure who is best positioned to review this, but maybe @corona10 could take a look?

Okay, I will take a look by this weekend :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 self-assigned this Mar 10, 2023
@artemmukhin

artemmukhin commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

@corona10 Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 requested changes Mar 11, 2023
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Would you like to separate the file into Modules/_testcapi/pyos.c
likewise https://github.com/python/cpython/blob/main/Modules/_testcapi/float.c ?

The test itself looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Mar 11, 2023
@bedevere-bot

bedevere-bot commented Mar 11, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@artemmukhin
Code review: extract tests to a separate file
6a2382e
@artemmukhin
artemmukhin requested a review from a team as a code owner March 11, 2023 15:56
@artemmukhin

artemmukhin commented Mar 11, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@corona10 Thank you for the review! I've separated the tests.

Although I haven't managed to run the tests from Modules/_testcapi on my machine. Before separating, I ran my tests via ./python -m test -v test_capi, but now they are not executed as I see. I found the C API Tests paragraph in the devguide, but I didn't find how to run these tests. Could you please clarify?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Mar 11, 2023
@bedevere-bot

bedevere-bot commented Mar 11, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from corona10 March 11, 2023 16:08
@artemmukhin
Code review: fix built-in test methods names
83e7f46
@artemmukhin

artemmukhin commented Mar 11, 2023

Copy link
Copy Markdown
Contributor Author

I've found out that the tests were not executed because of the wrong naming.

Test_testcapi only executes the test method if its built-in name starts with test_ and doesn't end with _code:

cpython/Lib/test/test_capi/test_misc.py

Lines 1422 to 1425 in ced13c9

class Test_testcapi(unittest.TestCase):
locals().update((name, getattr(_testcapi, name))
for name in dir(_testcapi)
if name.startswith('test_') and not name.endswith('_code'))

It is mentioned in the C API Tests paragraph to some extent:

Functions named test_* are used as tests directly

But at first reading, I got confused because in _testcapi/float.c file, the test_ prefix is only used in the C function name:

cpython/Modules/_testcapi/float.c

Lines 84 to 88 in 534660f

static PyMethodDef test_methods[] = {
{"float_pack", test_float_pack, METH_VARARGS, NULL},
{"float_unpack", test_float_unpack, METH_VARARGS, NULL},
{NULL},
};

However, in contrast to my tests, these test methods are actually executed through test_float.py:

cpython/Lib/test/test_float.py

Lines 1516 to 1519 in 534660f

class PackTests(unittest.TestCase):
def test_pack(self):
self.assertEqual(_testcapi.float_pack(2, 1.5, BIG_ENDIAN),
b'>\x00')

I hope this investigation might help other first-time contributors.

@corona10 If you think it would be helpful to clarify this point in the devguide, please let me know, and I'll be happy to open a corresponding PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Mar 12, 2023

Copy link
Copy Markdown
Member

I've found out that the tests were not executed because of the wrong naming.
Test_testcapi only executes the test method if its built-in name starts with test_ and doesn't end with _code:
But at first reading, I got confused because in testcapi/float.c file, the test prefix is only used in the C function name:
However, in contrast to my tests, these test methods are actually executed through test_float.py:

AFAIK, those things are intended.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 approved these changes Mar 12, 2023
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

But I want to listen to the opinion of @erlend-aasland for this testing structure.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Mar 12, 2023
@corona10
corona10 requested a review from erlend-aasland March 12, 2023 09:06
@artemmukhin

artemmukhin commented Mar 16, 2023

Copy link
Copy Markdown
Contributor Author

@erlend-aasland could you please take a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland approved these changes Mar 22, 2023
View reviewed changes
@artemmukhin

artemmukhin commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

Thank you!

Cc @corona10
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 merged commit 0a60dea into python:main Mar 22, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 22, 2023
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@artemmukhin @Fidget-Spinner
pythonGH-94808: Cover PyOS_mystrnicmp and PyOS_mystricmp (pythong…
e139442 
…h-102469)
@artemmukhin
artemmukhin deleted the cover-pystrcmp branch April 2, 2023 22:17
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@artemmukhin @warsaw
pythonGH-94808: Cover PyOS_mystrnicmp and PyOS_mystricmp (pythong…
f8c6f92 
…h-102469)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@corona10 corona10

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@artemmukhin @bedevere-bot @corona10 @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.