1164039 - Move test_TelemetryTimestamps.js to toolkit/telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Alessio Placitelli [:Dexter]]

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?

Comment 2

[Georg Fritzsche [:gfritzsche]]

We should move both test_TelemetryTimestamps.js & TelemetryTimestamps.jsm, not just the test.

Comment 3

[Vladan Djeric (:vladan)]

(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

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Sounds fine to me.

Comment 6

[Georg Fritzsche [:gfritzsche]]

ni?dexter for setting this up as a mentored bug.

Comment 7

[Alessio Placitelli [:Dexter]]

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

Comment 8

[Alessio Placitelli [:Dexter]]

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)

Comment 9

[Georg Fritzsche [:gfritzsche]]

Robert, would you be interested in this bug?

Comment 10

[rthyberg]

yeah sure thing.

Comment 11

[rthyberg]

Attachment: bug-1164039

Comment 12

[Alessio Placitelli [:Dexter]]

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?

Comment 13

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32be192c4a47

Comment 14

[rthyberg]

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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9f9d13c5a045

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f9d13c5a045