Closed Bug 1403902 Opened 2 years ago Closed 2 years ago

Migrate browser_webconsole_bug_580454_timestamp_l10n.js to the new frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

this might be better to have it as an xpcshell test (or mocha if we get to make l10 work in it)
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8946726 [details]
Bug 1403902 - migrate browser_webconsole_bug_580454_timestamp_l10n to xpcshell test;

https://reviewboard.mozilla.org/r/216694/#review222630

This looks good to me, thanks Julian !
The only question I have is if we should have this test in this filder or in the new-console-output one.
Eventually the webconsole/test folder will be replaced by the one in new-console when we delete the old code, so here I fear we might end up wiping out the xpcshell test as well.
This is probably unlikely, but having it in new-console-output would make the operation a no brainer. What do you think ?

::: devtools/client/webconsole/test/unit/test_webconsole_l10n.js:6
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +/* eslint no-unused-vars: [2, {"vars": "local"}] */

Why is it needed ? I don't see any variable not being used. Is it for run_test ?

::: devtools/client/webconsole/test/unit/test_webconsole_l10n.js:16
(Diff revision 1)
> +  ok(localizedString.indexOf(date.getHours()) != -1, "the localized " +
> +        "timestamp contains the hours");
> +  ok(localizedString.indexOf(date.getMinutes()) != -1, "the localized " +
> +        "timestamp contains the minutes");
> +  ok(localizedString.indexOf(date.getSeconds()) != -1, "the localized " +
> +        "timestamp contains the seconds");
> +  ok(localizedString.indexOf(date.getMilliseconds()) != -1, "the localized " +
> +        "timestamp contains the milliseconds");

nit: could we use `includes` instead of `indexOf` ? I find it slightly easier to read
Attachment #8946726 - Flags: review?(nchevobbe) → review+
Comment on attachment 8946726 [details]
Bug 1403902 - migrate browser_webconsole_bug_580454_timestamp_l10n to xpcshell test;

https://reviewboard.mozilla.org/r/216694/#review222630

Thanks! I wasn't sure if the test folder was supposed to stay under new-console-output or not. If it's going up one level after the migration then it makes sense to have the "unit" folder here.

> Why is it needed ? I don't see any variable not being used. Is it for run_test ?

Good catch, removed.

> nit: could we use `includes` instead of `indexOf` ? I find it slightly easier to read

Sure! This was a leftover from the initial test. For clarity I also amended to perform a hg move rather than a handmade copy.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/811278d126bb
migrate browser_webconsole_bug_580454_timestamp_l10n to xpcshell test;r=nchevobbe
Great !
https://hg.mozilla.org/mozilla-central/rev/811278d126bb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.