Enhance TestDateTimeFormat.cpp to test requests for a non-en-US locale

RESOLVED FIXED in Firefox 55

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
+++ This bug was initially created as a clone of Bug #1351427 +++

Enhance TestDateTimeFormat.cpp to test that requests for a non-en-US locale.

In bug 1351427 we changed C++ date/time formatting to that it respects the OS pattern. This is only tested for en-US on a test server with en-US system/user locale.

Enhance this test by requesting the OS pattern for a de-DE locale on the same en-US test server.

Expected result: de-DE patterns need to be used, not the en-US pattern.

The code in GetWindowsLocaleFor() should already do that.
(Assignee)

Updated

a year ago
Summary: Enhance TestDateTimeFormat.cpp to test that requests for a non-en-US locale → Enhance TestDateTimeFormat.cpp to test requests for a non-en-US locale
(Assignee)

Updated

a year ago
No longer depends on: 1339650
(Assignee)

Comment 1

a year ago
Created attachment 8856985 [details] [diff] [review]
1355465-non-US-locale.patch (v1).

This works nicely on Windows with an en-US OS and en-US format locale.

We request de-DE and we get the de-DE Windows OS formats, not the en-US formats. This is how it's meant to work. Good job, Zibi!

Now let's see whether this already works on Linux or Mac, there are less variations to German formats, so it might already pass.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ac9f2c9fd53a2a6638dfae5133dcfcae94fb203
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Comment hidden (spam)
(Assignee)

Comment 3

a year ago
OK, didn't work, Linux and Mac still append "GMT", so let's strip that.
(Assignee)

Comment 4

a year ago
Created attachment 8857074 [details] [diff] [review]
1355465-non-US-locale.patch (v2).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e92da53aacef2fc1c9618bef6c42d41a496ff60
This passed on Linux now, I expect Mac to pass as well.
Attachment #8856985 - Attachment is obsolete: true
Attachment #8857074 - Flags: review?(VYV03354)
Attachment #8857074 - Flags: feedback?(gandalf)
Comment on attachment 8857074 [details] [diff] [review]
1355465-non-US-locale.patch (v2).

Review of attachment 8857074 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good! I have one design suggestion for you to evaluate.

::: intl/locale/tests/gtest/TestDateTimeFormat.cpp
@@ +122,5 @@
> +  int32_t ind = datetime.Find(" GMT");
> +  if (ind != kNotFound)
> +    datetime.Truncate(ind);
> +
> +  // Strip leading "Donnerstag, " or "Mittwoch, " (found on Windows).

I think I'm a bit afraid of writing a test in a way that requires per-platform adjustments.

Can we instead of testing for string equality, test if certain bits are included?

Like, the date should have "02" or "March", but it doesn't matter if there was a soft or hard comma separator.
If you want to put more time into it, you could also test for order (month should  come after day or reverse), but I'd prefer to keep the output not very strictly tested since it may evolve (next release of CLDR may adjust some formattings etc.).
Attachment #8857074 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 6

a year ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> Can we instead of testing for string equality, test if certain bits are
> included?
So suggested that in bug 1351427 comment #57 ("make partial tests like"). I read and remember your comments ;-)
Overall that is harder to do, so it's just plain laziness to whip up a test quickly.
Attachment #8857074 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 7

a year ago
This depends on bug 1351427 which needs to be checked in first.
Successful try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d2adb2e0bc0030286b41b9e3fe5dc85f90aca3
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1ca47045ab08
Enhance TestDateTimeFormat.cpp with non-en-US locale. r=emk
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ca47045ab08
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.