Attempt to further increase test coverage of calendar module #69714
Comments
|
Opened to submit a patch.
|
RohitMediratta
mannequin
added
tests
|
Rohit, it looks like your patch is reversed. The lines with a + sign already exist; are the - lines your proposed additions? Equivalently, revision f7db966c9fee already exists; is fb9d4ccbadf0 your new proposed revision? Assuming the patch is reversed, I suggest keeping the locale=None case, perhaps as a separate test case or loop iteration. Otherwise you are throwing out one test case to add another. |
|
Thanks for the comments. I did indeed have the patch reversed. I've resolved it here. Martin: I had the locale=None case in the patch. |
|
no problem about the second patch of Rohit. pass the test with default and I have tested the code in the REPL. |
|
The problem with Rohit's patch is that it throws out existing test case. |
|
Sure, But the patch is correct. Now, you are right, we have to ask him a new patch where the function is really tested. |
|
The patch needs to be reviewed. If the tests are still relevant and increase coverage, it needs to be converted to a GitHub PR. Otherwise this issue can be closed. See also bpo-13330. |
|
This hasn't been converted into a PR, if someone would like to we can re-open this issue in the future. A |
….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 locale
So far case (2) is covered and case (1) is in 78935da (same branch).
This commit adds a remaining case (3).
[1] In the current implementation, this translates into the following
approach:
GET current locale
IF current locale == "C" THEN
SET current locale TO ""
GET current locale
ENDIF
This condition cannot be true. `_locale.setlocale()` from the C module raises `locale.Error` instead of returning `None` for `different_locale.__enter__` (where `self.oldlocale` is set).
|
Reopening as a new PR has appeared (#93655). A |
…verage)" This reverts commit a96786c.
No need to process the function. This also helps in supporting CLI testing.
These tests are now part of `test_illegal_arguments`. This reverts commit 238c527.
|
@bxsx Please leave the issue number off the commit message for all PR branch commits after the first. It spams the issue with unneeded info that is also on the PR itself. |
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 locale
So far case (2) is covered and case (1) is in 78935da (same branch).
This commit adds a remaining case (3).
[1] In the current implementation, this translates into the following
approach:
GET current locale
IF current locale == "C" THEN
SET current locale TO ""
GET current locale
ENDIF
* Remove unreachable code (and increase test coverage)
This condition cannot be true. `_locale.setlocale()` from the C module
raises `locale.Error` instead of returning `None` for
`different_locale.__enter__` (where `self.oldlocale` is set).
* Expand the try clause to calls to `LocaleTextCalendar.formatmonthname()`.
This method temporarily changes the current locale to the given locale,
so `_locale.setlocale()` may raise `local.Error`.
Co-authored-by: Rohit Mediratta <[email protected]>
Co-authored-by: Jessica McKellar <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Irit Katriel <[email protected]>
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
calendarmodule fully tested #93655The text was updated successfully, but these errors were encountered: