Move test_TelemetryTimestamps.js to toolkit/telemetry

RESOLVED FIXED in Firefox 45

Status

()

P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Dexter, Assigned: robertthyberg, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox41 affected, firefox45 fixed)

Details

(Whiteboard: [measurement:client][lang=js][good next bug])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
test_TelemetryTimestamps.js [1] lives in toolkit/modules even though it's testing Telemetry stuff. I think this should probably live in toolkit/telemetry.

On a first look, there doesn't seem to be anything which ties the test to the toolkit/modules folder.

[1] - https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/toolkit/modules/tests/xpcshell/test_TelemetryTimestamps.js
(Reporter)

Updated

4 years ago
Blocks: 1122482
(Reporter)

Comment 1

4 years ago
Vladan, what do you think about moving this test to toolkit/telemetry? Can you think of any particular reason why this should stay in toolkit/modules?
Flags: needinfo?(vdjeric)
We should move both test_TelemetryTimestamps.js & TelemetryTimestamps.jsm, not just the test.
(In reply to Alessio Placitelli [:Dexter] from comment #1)
> Vladan, what do you think about moving this test to toolkit/telemetry? Can
> you think of any particular reason why this should stay in toolkit/modules?

Seems ok to me, maybe check with Gavin. He wrote the test https://hg.mozilla.org/mozilla-central/rev/383712b389bc
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)
> (In reply to Alessio Placitelli [:Dexter] from comment #1)
> > Vladan, what do you think about moving this test to toolkit/telemetry? Can
> > you think of any particular reason why this should stay in toolkit/modules?
> 
> Seems ok to me, maybe check with Gavin. He wrote the test
> https://hg.mozilla.org/mozilla-central/rev/383712b389bc

Gavin, any objections?
We keep overlooking that those files (e.g. when running tests toolkit/components/telemetry/test).
Flags: needinfo?(gavin.sharp)
Sounds fine to me.
Flags: needinfo?(gavin.sharp)
Blocks: 1137262
No longer blocks: 1122482
ni?dexter for setting this up as a mentored bug.
Flags: needinfo?(alessio.placitelli)
Priority: -- → P3
Whiteboard: [measurement:client]
(Reporter)

Comment 7

3 years ago
This bug is about moving TelemetryTimestamps.jsm [0] and its tests [1] from "toolkit/modules/" to "toolkit/components/telemetry". This could be done by:

1 - Moving the TelemetryTimestamps.jsm file from toolkit/modules to toolkit/components/telemetry
2 - Removing its entry from toolkit/modules/moz.build [2] and adding it to toolkit/components/telemetry/moz.build
3 - Moving test_TelemetryTimestamps.js from toolkit/modules/tests/xpcshell/ to toolkit/components/telemetry/tests/unit/
4 - Removing its entry from  toolkit/modules/tests/xpcshell/xpcshell.ini [3] and adding it to toolkit/components/telemetry/tests/unit/xpcshell.ini
5 - Deal with test fallout (but there shouldn't be any).

Please note that files should be moved with the Mercurial command "hg mv <source> <dest>" so we don't loose the history for the moved file.

[0] - https://dxr.mozilla.org/mozilla-central/rev/f029ccdee154bdc2b49a1e0fcd5b0fa0397aa97b/toolkit/modules/TelemetryTimestamps.jsm
[1] - https://dxr.mozilla.org/mozilla-central/rev/f029ccdee154bdc2b49a1e0fcd5b0fa0397aa97b/toolkit/modules/tests/xpcshell/test_TelemetryTimestamps.js
[2] - https://dxr.mozilla.org/mozilla-central/rev/f029ccdee154bdc2b49a1e0fcd5b0fa0397aa97b/toolkit/modules/moz.build#76
[3] - https://dxr.mozilla.org/mozilla-central/rev/f029ccdee154bdc2b49a1e0fcd5b0fa0397aa97b/toolkit/modules/tests/xpcshell/xpcshell.ini#56
Mentor: alessio.placitelli
Flags: needinfo?(alessio.placitelli)
Whiteboard: [measurement:client] → [measurement:client][lang=js][good next bug]
(Reporter)

Comment 8

3 years ago
In order to confirm that things are working correctly, the following tests should be run:

- ./mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetryTimestamps.js
- ./mach xpcshell-test toolkit/components/telemetry
- ./mach xpcshell-test toolkit/modules/tests/xpcshell/test_TelemetryTimestamps.js (this MUST fail, as the test file should no longer be there)
Robert, would you be interested in this bug?
Flags: needinfo?(robertthyberg)
(Assignee)

Comment 10

3 years ago
yeah sure thing.
Flags: needinfo?(robertthyberg)
(Assignee)

Comment 11

3 years ago
Created attachment 8684719 [details] [diff] [review]
bug-1164039
Attachment #8684719 - Flags: review?(alessio.placitelli)
(Reporter)

Updated

3 years ago
Assignee: nobody → robertthyberg
Status: NEW → ASSIGNED
(Reporter)

Comment 12

3 years ago
Comment on attachment 8684719 [details] [diff] [review]
bug-1164039

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

Thanks, this looks good. Did you make sure all tests run locally without any problem as stated in comment 8?
Attachment #8684719 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 14

3 years ago
Comment on attachment 8684719 [details] [diff] [review]
bug-1164039

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryTimestamps.js
@@ +23,5 @@
>  var gGlobalScope = this;
>  function loadAddonManager() {
>    let ns = {};
>    Cu.import("resource://gre/modules/Services.jsm", ns);
> +  let head = "../../../../mozapps/extensions/test/xpcshell/head_addons.js";

This was the only line that needed to be changed to make the first 2 tests work. The 3rd test gave an error but it passed with an ok because no test could be found.
(Reporter)

Updated

3 years ago
Keywords: checkin-needed

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9f9d13c5a045
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.