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.

Sign up for GitHub

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

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

Merged
merged 8 commits into from
Jul 18, 2019
Merged

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2e8cc0a
bpo-36511 skip strptime/strftime cp65001 failures
Jun 28, 2019
a805590
add bpo-37552 comments
Jul 10, 2019
6cfcddb
refactor skips
Jul 17, 2019
323f283
merge upstream
Jul 17, 2019
ce72404
fix bad merge
Jul 17, 2019
7665b54
fix bad fix of bad merge
Jul 17, 2019
ccde057
commas are important too
Jul 17, 2019
e8001cb
include strftime/strptime in skip
Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Lib/test/test_strptime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ def test_pattern_escaping(self):
"%s does not have re characters escaped properly" %
pattern_string)

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment referencing bpo-36511 here, so people in the future will know when it is safe to stop skipping this?

It might even be worth opening a separate BPO issue for tracking this specific thing, because issue 36511 seems like more of an omnibus issue.

@zooba You probably know more about the lifetime of Windows support here - what is the best way to communicate that it will be safe to remove this skipIf?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, a specific issue for this particular failure would be good. There was one already for the same thing (to do with passing de_DE instead of de-DE IIRC, which triggered the experimental/broken UTF-8 support in the UCRT) that might be okay to reuse.

I don't know how best to communicate a future time for this, especially since the UCRT may get updated down-level (in earlier releases) and certain fixes may release out of band. Probably best to just leave a comment with the current UCRT version (@paulmon?) and a reminder to verify as new updates are made.

Copy link
Contributor Author

@paulmon paulmon Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@zooba, Is this the same issue? https://bugs.python.org/issue36792
These tests aren't crashing as indicated in bpo-36792, but they do cause the Windows ARM32 buildbot to fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a new issue: https://bugs.python.org/issue37552 and added comments.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than repeating the whole condition and comment several times, I'd rather see it factored out to a simple @skip_if_buggy_ucrt decorator.

Separately, I would suggest that that decorator should actually check for one of the known bugs in the UCRT, and only skip if that bug is still present; we will forget to remove these skips later otherwise :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are both great ideas :)

'issue in MSVC UCRT')
def test_compile(self):
# Check that compiled regex is correct
found = self.time_re.compile(r"%A").match(self.locale_time.f_weekday[6])
Expand Down Expand Up @@ -365,6 +367,8 @@ def test_bad_offset(self):
_strptime._strptime("-01:3030", "%z")
self.assertEqual("Inconsistent use of : in -01:3030", str(err.exception))

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_timezone(self):
# Test timezone directives.
# When gmtime() is used with %Z, entire result of strftime() is empty.
Expand Down Expand Up @@ -489,6 +493,8 @@ class CalculationTests(unittest.TestCase):
def setUp(self):
self.time_tuple = time.gmtime()

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_julian_calculation(self):
# Make sure that when Julian is missing that it is calculated
format_string = "%Y %m %d %H %M %S %w %Z"
Expand All @@ -498,6 +504,8 @@ def test_julian_calculation(self):
"Calculation of tm_yday failed; %s != %s" %
(result.tm_yday, self.time_tuple.tm_yday))

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_gregorian_calculation(self):
# Test that Gregorian date can be calculated from Julian day
format_string = "%Y %H %M %S %w %j %Z"
Expand All @@ -512,6 +520,8 @@ def test_gregorian_calculation(self):
self.time_tuple.tm_year, self.time_tuple.tm_mon,
self.time_tuple.tm_mday))

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_day_of_week_calculation(self):
# Test that the day of the week is calculated as needed
format_string = "%Y %m %d %H %S %j %Z"
Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_time.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,8 @@ def test_default_values_for_zero(self):
result = time.strftime("%Y %m %d %H %M %S %w %j", (2000,)+(0,)*8)
self.assertEqual(expected, result)

@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_strptime(self):
# Should be able to go round-trip from strftime to strptime without
# raising an exception.
Expand Down Expand Up @@ -673,6 +675,8 @@ class TestStrftime4dyear(_TestStrftimeYear, _Test4dYear, unittest.TestCase):

class TestPytime(unittest.TestCase):
@unittest.skipUnless(time._STRUCT_TM_ITEMS == 11, "needs tm_zone support")
@unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',
'issue in MSVC UCRT')
def test_localtime_timezone(self):

# Get the localtime and examine it for the offset and zone.
Expand Down