Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-69714: Makecalendar module fully tested#93655

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ambv merged 29 commits intopython:mainfrombxsx:gh-69714/calendar-test-coverage
Jul 22, 2023

Conversation

@bxsx
Copy link
Contributor

@bxsxbxsx commentedJun 9, 2022
edited by bedevere-bot
Loading

Summary

Resolves#69714 by increasing test coverage ofcalendar module to100%[1].

Coverage reports

Before

main branch test coverage report forcalendar module:

$ ./python.exe coveragepy report --show-missingName              Stmts   Miss  Cover   Missing-----------------------------------------------Lib/calendar.py     399     60    85%   559, 564-570, 582, 602, 664-764, 768-----------------------------------------------TOTAL               399     60    85%

After

The PR branch test coverage report forcalendar module:

$ ./python.exe coveragepy report --show-missingName              Stmts   Miss  Cover   Missing-----------------------------------------------Lib/calendar.py     397      3    99%   722, 742, 766-----------------------------------------------TOTAL               397      3    99%

Footnotes

[1] The remaining three lines are:

$ cat -n Lib/calendar.py | sed -n '722p;742p;766p'   722        sys.exit(1)   742            sys.exit(1)   766    main()$

and they are,in fact, already covered by tests (only not reported by coveragepy).

rohitmedirattaand others added11 commitsJune 6, 2022 02:29
….Locale{Text|HTML}Calendar`There are 3 paths to use `locale` argument in`calendar.Locale{Text|HTML}Calendar.__init__(..., locale=None)`:(1) `locale=None` -- denotes the "default locale"[1](2) `locale=""` -- denotes the native environment(3) `locale=other_valid_locale` -- denotes a custom localeSo far case (2) is covered and case (1) is in78935da (same branch).This commit adds a remaining case (3).[1] In the current implementation, this translates into the followingapproach:GET current localeIF current locale == "C" THEN  SET current locale TO ""  GET current localeENDIF
This condition cannot be true. `_locale.setlocale()` from the C moduleraises `locale.Error` instead of returning `None` for`different_locale.__enter__` (where `self.oldlocale` is set).
This patch has already been applied to `main` branch (viapythongh-93468), butwith wrong copyright. After merging this commit to `main`, git `Author`metadata will be updated to the original author.
@ghost
Copy link

ghost commentedJun 9, 2022
edited by ghost
Loading

The following commit authors need to sign the Contributor License Agreement:

Click the button to sign:
CLA not signed

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

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

Also cc:@hugovk (who I know has different opinions on testing to me!)

A

@AA-TurnerAA-Turner added stdlibStandard Library Python modules in the Lib/ directory type-featureA feature request or enhancement testsTests in the Lib/test dir labelsJun 9, 2022
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@AA-Turner
Copy link
Member

You will need a NEWS entry, along the lines ofAdded tests to :mod:`calendar` to achieve full test coverage.

A

@bxsx
Copy link
ContributorAuthor

bxsx commentedJun 9, 2022
edited
Loading

@AA-Turner
You will need a NEWS entry, along the lines ofAdded tests to :mod:`calendar` to achieve full test coverage.

Thanks! I just added it.

Another problem is the lack of CLA signatures from@jesstess and@rohitmediratta. The applied patches were quite old, so I'm not sure the email addresses are up to date (I don't know if the bot is sending emails about the CLA). Anyway, it looks like GitHub has assigned these commits to the correct usernames, so I'm tagging Jessica and Rohit in this comment to help notify :)

@AA-Turner
Copy link
Member

You'll need to fix the Ubuntu failures intest_locale_calendar_formatmonthname too:

Traceback (most recent call last):  File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_calendar.py", line650, intest_locale_calendar_formatmonthnameself.assertEqual(cal.formatmonthname(2022,6,2,withyear=False),"June")^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/calendar.py", line588, informatmonthnamewith different_locale(self.locale):^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/calendar.py", line555, in__enter__    _locale.setlocale(_locale.LC_TIME,self.locale)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/locale.py", line626, insetlocalereturn _setlocale(category, locale)^^^^^^^^^^^^^^^^^^^^^^^^^^^^locale.Error:unsupported locale setting

This may be another test-skipping scenario, I don't know what the values it is trying to set are though so I can't guess much further.

A

@AA-Turner
Copy link
Member

I'm tagging Jessica and Rohit in this comment to help notify

You might also want to consider emailing the two of them if you don't hear back, neither seem to have much recent publicly shown activity on GH.

A

bxsx reacted with thumbs up emoji

@bxsx
Copy link
ContributorAuthor

bxsx commentedJun 9, 2022
edited
Loading

@AA-Turner
You'll need to fix the Ubuntu failures intest_locale_calendar_formatmonthname too:

This may be another test-skipping scenario, I don't know what the values it is trying to set are though so I can't guess much further.

Or maybe (based on the traceback) it's related to one of the corner cases from#93655 (comment)?
I don't have access to Ubuntu at the moment but I can revert that change and see what happens before further investigation.
This is a newly added test BTW.

@AA-Turner
Copy link
Member

I can revert that change and see what happens before further investigation.

Sadly it doesn't seem to have worked :(

A

…()`.This method temporarily changes the current locale to the given locale,so `_locale.setlocale()` may raise `local.Error`.
… test coverage)""This reverts commit40e0da3.So it brings back commita96786c.
@bxsx
Copy link
ContributorAuthor

Sadly it doesn't seem to have worked :(

Yes, I misread the traceback in a rush before disconnecting, but I wanted to kick off builds to get results when I get back. Time saving ;)

For now, I have restoreda96786c.

However, it is true that the cause of the problem is related to#31214 and as you correctly predicted, this is anothertest-skipping scenario. Fixed in2b6f1f9.

@bxsx
Copy link
ContributorAuthor

test_illegal_arguments andtest_too_many_arguments fail in Azure Pipelines PR.
I will try to investigate it, but for now I'm marking this PR as a draft.

@bxsxbxsx marked this pull request as draftJune 10, 2022 02:05
@bxsx
Copy link
ContributorAuthor

test_illegal_arguments andtest_too_many_arguments fail in Azure Pipelines PR.
I will try to investigate it, but for now I'm marking this PR as a draft.

Fixed in8b7bc06.

@bxsx
Copy link
ContributorAuthor

Ok, all comments are now addressed and resolved. All tests pass successfully. ✔️
Still missing CLA signatures from@jesstess and@rohitmediratta. I will email them directly. 🤞

I think the PR is ready for another round of reviews ;-)

@bxsxbxsx marked this pull request as ready for reviewJune 11, 2022 19:06
Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

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

The updated tests look good, I'll wait for Hugo's thoughts.

A

bxsx reacted with thumbs up emoji
@bxsxbxsx requested review fromAA-Turner andhugovkJune 12, 2022 17:01
@bxsx
Copy link
ContributorAuthor

Good news for the weekend.

Rohit signed the CLA. Thanks again@rohitmediratta!

I also looked at@jesstess GH profile and I believe the missing CLA has already been signed by Jessica as she is aformer director of the Python Software Foundation and a member of@python.

I've emailedcontributions@python.org to sort this out.

AA-Turner, hugovk, and arhadthedev reacted with thumbs up emoji

@terryjreedy
Copy link
Member

Here is the holdup situation:
Forhttps://bugs.python.org/issue13330, Attempt full test coverage of LocaleTextCalendar.formatweekday
Sean Fleming, no CLA recorded, uploaded a patch 2011-11-03 02:14.
bpo user jesstess Jessica McKellar,https://bugs.python.org/user11041, signed the CLA "on: 2013-04-12.20:00:00".
She more or less rejected Sean's patch and uploaded a much different patch,
https://bugs.python.org/file35060/issue13330.patch on 2014-04-27 16:43.
Serhiy Storchaka mentioned a couple of problems.

When moved to gh, issue became#57539. Irit Katriel requested gh PR and mentioned this issue.
Last June, hugovk converted Jessica's patch to a PR, with the correction requested by Serhiy.
The commit message mistakenly said "Co-authored-by: Sean Fleming". This apparently did not trigger the CLA bot of the time to request a signature from Sean. ambv merged and backported on June 7, 2022.

On June 9, bxsx addedab069a4, which added Jessica's original code, without the Serhiy correction. It somehow credited Jessica as author. Fortunately, the deletion of the correction is somehow not in the merged patch. Unfortunately, it fooled the CLA bot into thinking that she was an author of this PR.

@ambv This PR, which has had a lot of work by multiple people, is blocked by the false 'need' for an unobtainable signature from@jesstess Jessica McKeller, who did not contribute to this PR. Should reverting ab069a4 fix the mistake or is something else needed. If so, can you override the bot? (Though a final review may be needed.)

@ambvambv merged commit2aba047 intopython:mainJul 22, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments

@StanFromIrelandStanFromIrelandStanFromIreland left review comments

@hugovkhugovkAwaiting requested review from hugovk

@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner

Assignees

No one assigned

Labels

stdlibStandard Library Python modules in the Lib/ directorytestsTests in the Lib/test dirtype-featureA feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Attempt to further increase test coverage of calendar module

11 participants

@bxsx@bedevere-bot@AA-Turner@terryjreedy@vstinner@hugovk@StanFromIreland@ambv@rohitmediratta@jesstess@iritkatriel

[8]ページ先頭

©2009-2025 Movatter.jp