Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139413005:
Add tests for TimeRemainingLong() and TimeDurationLong() (Closed)
|
Created:
6 years, 10 months ago by Thiemo Nagel Modified:
6 years, 9 months ago CC:
chromium-reviews, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionNot going to land -- superseded by https://codereview.chromium.org/147443007/
Add tests for TimeRemainingLong() and TimeDurationLong()
BUG=none
TEST=improves test coverage
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Re-formatted according to Bartosz' suggestions #
Total comments: 8
Patch Set 3 : More fixes/improvements, as suggested by Bartosz #
Total comments: 1
Patch Set 4 : fresh attempt following Jungshik's review #Patch Set 5 : Rebased #Patch Set 6 : Variant with less line wrapping #Patch Set 7 : Rebased #Messages
Total messages: 10 (0 generated)
Hi Bartosz, could you please take a look? Thanks, Thiemo
https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:21: const base::string16 expect(ASCIIToUTF16(expect_ascii)); Nit: s/expect/expected/ https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:22: const base::string16 sec (ASCIIToUTF16("sec")); * Do not align with spaces. * Please add one_ prefixes to sec and min https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:24: const base::string16 ond (ASCIIToUTF16("ond")); "ond" and "ute" are not exactly self-explanatory. Does it really make sense to define them here as constants? Why not create the string16s on the fly where you need them? https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:26: const base::string16 _ago (ASCIIToUTF16(" ago")); Do not start variable names with _. I know it stands for a space here but it looks like something that implies a significant meaning. Just call the variables "ago" and "left" or, better yet, create the string16s on the fly where you need them. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:29: base::string16 expect_long = expect; // sec, min --> second, minute Do not align with spaces. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:33: expect_long.insert(expect_long.find(sec)+sec.length(), ond); Here and further down: Spaces around the + are missing. Nit: No {} for single-line conditionals. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:42: const base::string16 expect_elapsed = expect + _ago; Here and throughout the rest of the CL: Do not align with spaces.
Could you please take another look? Thanks, Thiemo https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:21: const base::string16 expect(ASCIIToUTF16(expect_ascii)); On 2014/01/29 15:46:16, bartfab wrote: > Nit: s/expect/expected/ Done. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:22: const base::string16 sec (ASCIIToUTF16("sec")); On 2014/01/29 15:46:16, bartfab wrote: > * Do not align with spaces. > > * Please add one_ prefixes to sec and min Done, except "one_" prefixes. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:24: const base::string16 ond (ASCIIToUTF16("ond")); On 2014/01/29 15:46:16, bartfab wrote: > "ond" and "ute" are not exactly self-explanatory. Does it really make sense to > define them here as constants? Why not create the string16s on the fly where you > need them? Done. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:26: const base::string16 _ago (ASCIIToUTF16(" ago")); On 2014/01/29 15:46:16, bartfab wrote: > Do not start variable names with _. I know it stands for a space here but it > looks like something that implies a significant meaning. Just call the variables > "ago" and "left" or, better yet, create the string16s on the fly where you need > them. Done. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:29: base::string16 expect_long = expect; // sec, min --> second, minute On 2014/01/29 15:46:16, bartfab wrote: > Do not align with spaces. Done. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:33: expect_long.insert(expect_long.find(sec)+sec.length(), ond); On 2014/01/29 15:46:16, bartfab wrote: > Here and further down: Spaces around the + are missing. > > Nit: No {} for single-line conditionals. Done. https://codereview.chromium.org/139413005/diff/60001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:42: const base::string16 expect_elapsed = expect + _ago; On 2014/01/29 15:46:16, bartfab wrote: > Here and throughout the rest of the CL: Do not align with spaces. Done.
https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:24: const base::string16 ond(ASCIIToUTF16("ond")); Nit: No longer used. https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:25: const base::string16 ute(); Nit: No longer used and actually completely wrong as ute == "" now, not ute == "ute" :) https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:36: if (expected_long_minute.find(min) != base::string16::npos) Nit: Not sure whether it would make things cleaner/easier, but you could remove one of the blocks if you did the substitution in the opposite order: * take |expected| and s/min/minute/, producing |expected_long_minute| * take |expected_long_minute| and s/sec/seconds/, producing |expected_long| https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:45: EXPECT_EQ(expected_long, TimeFormat::TimeDurationLong(delta)); Is there a reason that TimeDurationLong() expands "min" and "sec" but TimeRemainingLong() expands "min" only? It seems awfully inconsistent.
Hi Jungshik, I've extended test coverage in time_format_unittest.cc to include TimeRemainingLong() and TimeDurationLong(). Could you please have a look? Thank you and best regards, Thiemo https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:24: const base::string16 ond(ASCIIToUTF16("ond")); On 2014/01/31 12:58:23, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:25: const base::string16 ute(); On 2014/01/31 12:58:23, bartfab wrote: > Nit: No longer used and actually completely wrong as ute == "" now, not ute == > "ute" :) That's embarrassing. I forgot to remove ond and ute. It seems that the compiler warns about simple types that are unused, but not objects. Too bad. https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:36: if (expected_long_minute.find(min) != base::string16::npos) On 2014/01/31 12:58:23, bartfab wrote: > Nit: Not sure whether it would make things cleaner/easier, but you could remove > one of the blocks if you did the substitution in the opposite order: > > * take |expected| and s/min/minute/, producing |expected_long_minute| > * take |expected_long_minute| and s/sec/seconds/, producing |expected_long| Good idea! I have the impression that this makes the code more readable. https://codereview.chromium.org/139413005/diff/70001/ui/base/l10n/time_format... ui/base/l10n/time_format_unittest.cc:45: EXPECT_EQ(expected_long, TimeFormat::TimeDurationLong(delta)); On 2014/01/31 12:58:23, bartfab wrote: > Is there a reason that TimeDurationLong() expands "min" and "sec" but > TimeRemainingLong() expands "min" only? It seems awfully inconsistent. I fully agree. However, this behaviour is documented in the header file, so I assume it's intentional: // Returns times in remaining-long-format: "3 minutes left", "2 days left". // Currently, this only affects the minutes in long format, the rest // of the time units are formatted the same as TimeRemaining does.
https://codereview.chromium.org/139413005/diff/110001/ui/base/l10n/time_forma... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/139413005/diff/110001/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:35: ASCIIToUTF16("ond")); This string manipulation is really hacky, I'm afraid. Instead, it'd be much cleaner to add a separate helper function for long or extend TestTimeFormats to accept a parameter to specify 'long' or 'short'. Then, TimeFormat:FormatTime can call TestTimeFormats with different format indicator (long, short). Actually, the current version of TestTimeFormats also does something not very expandable to non-English locales by appending 'left' and 'ago'. While you're at it, you may as well parameterize it, too. It'd make # of lines in FormatTime test 6 times as many as the current. If that's a problem, you can also make a loop around them to keep the # of lines the same.
I've uploaded a new attempt: One test function per TimeFormat method, very straight-forward, no hacks, easy to extend. What do you think?
On 2014/02/03 22:21:27, Thiemo Nagel wrote: > I've uploaded a new attempt: One test function per TimeFormat method, very > straight-forward, no hacks, easy to extend. What do you think? LGTM I usually set up a 'giant' array of structs with input params and expected outputs and go through them in a loop or two (and add '<< "input=" << input_param' to EXPECT_FOO()), but what you have done is perfectly fine, too.
> I usually set up a 'giant' array of structs with input params and expected > outputs and go through them in a loop or two (and add '<< "input=" << > input_param' to EXPECT_FOO()), but what you have done is perfectly fine, too. Could you please point me to an example? I don't like the way the lines wrap in my code, but I'm lacking a good idea to improve it.
Just uploaded another variant that has much less line wrappings. What do you think? |
