1197612 - test_TelemetrySendOldPings.js test failure due to assumptions about FHR

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[aleth [:aleth]]

test_TelemetrySendOldPings.js has been failing on comm-central:
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js | test_recent_pings_sent - [test_recent_pings_sent : 117] 0 == 4 

It turns out this is because the tests assume the pref datareporting.healthreport.uploadEnabled is set to true by default (if it is not, sendingEnabled() in TelemetrySend.jsm returns false). 

One of the tests, test_pendingPingsQuota, temporarily sets the pref to false and sets it back to true at the end.

However, this is only the case if MOZ_SERVICES_HEALTHREPORT is defined. This shouldn't be assumed, and is not the case e.g. for Thunderbird.

Comment 1

[aleth [:aleth]]

Attachment: telemetrytestfix.diff

Comment 2

[aleth [:aleth]]

This patch also fixes a strict warning that occurs when aPingInfos[type].age is undefined.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8651526 [details] [diff] [review]
telemetrytestfix.diff

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

Thanks - the change described looks good, i don't think we need the other changes though.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +68,4 @@
>          // savePing writes to the file synchronously, so we're good to
>          // modify the lastModifedTime now.
>          let filePath = getSavePathForPingId(pingId);
> +        yield File.setDates(filePath, null, now - age);

What are these changes good for?
Are these left-overs?

@@ +564,5 @@
>  add_task(function* teardown() {
>    yield PingServer.stop();
> +
> +  // Restore default pref value.
> +  Services.prefs.clearUserPref(PREF_FHR_UPLOAD);

We don't need to reset state in xpcshell tests, each test gets its own profile environment.

Comment 4

[aleth [:aleth]]

Attachment: telemetrytestfix.diff v2

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
> @@ +68,4 @@
> >          // savePing writes to the file synchronously, so we're good to
> >          // modify the lastModifedTime now.
> >          let filePath = getSavePathForPingId(pingId);
> > +        yield File.setDates(filePath, null, now - age);
> 
> What are these changes good for?
> Are these left-overs?

As per comment 2, it was just a drive-by change to get rid of this strict warning:

CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property aPingInfos[type].age" {file: "/Users/helvellyn/buildhg/mc/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js" line: 62}]

No functional difference though, so I took it out as requested.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8651526 [details] [diff] [review]
telemetrytestfix.diff

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

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +60,5 @@
>    let now = Date.now();
>  
>    for (let type in aPingInfos) {
>      let num = aPingInfos[type].num;
> +    let age = aPingInfos[type].age;

Ah, i saw the comment for this now. Lets just change this to:
> now - (aPingInfos[type].age || 0)

Comment 6

[aleth [:aleth]]

Attachment: telemetrytestfix.diff v3

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)

> Ah, i saw the comment for this now. Lets just change this to:
> > now - (aPingInfos[type].age || 0)

Done.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/83f63500ab50

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83f63500ab50

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8651722 [details] [diff] [review]
telemetrytestfix.diff v3

Approval Request Comment
[Feature/regressing bug #]: telemetry tests
[User impact if declined]: comm xpcshell tests continue to fail
[Describe test coverage new/current, TreeHerder]:
c-c doesn't have this failure: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e6c5bce2b8fb
m-c mostly starred, remaining failures look unrelated: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=04b8c412d9f5

[Risks and why]: no expected risks
[String/UUID change made/needed]:

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Georg, you marked it as wontfix for 42 and Philipp is asking for an uplift, do you agree?

Comment 11

[Georg Fritzsche [:gfritzsche]]

I didn't see a need for uplift, but Philipp explained to me on IRC that comm-aurora/beta are based on mozilla-aurora/-beta, so we want to do this.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8651722 [details] [diff] [review]
telemetrytestfix.diff v3

OK, let's take it then.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4386fc15cc9d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0519004d585e