Closed Bug 1197612 Opened 4 years ago Closed 4 years ago

test_TelemetrySendOldPings.js test failure due to assumptions about FHR

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch telemetrytestfix.diff (obsolete) — Splinter Review
Attachment #8651526 - Flags: review?(gfritzsche)
This patch also fixes a strict warning that occurs when aPingInfos[type].age is undefined.
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.
Attachment #8651526 - Flags: review?(gfritzsche)
Assignee: nobody → aleth
Blocks: 1122482
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Attached patch telemetrytestfix.diff v2 (obsolete) — Splinter Review
(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.
Attachment #8651526 - Attachment is obsolete: true
Attachment #8651718 - Flags: review?(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)
Attachment #8651526 - Attachment is obsolete: false
Attachment #8651718 - Flags: review?(gfritzsche) → review+
Attachment #8651526 - Attachment is obsolete: true
(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.
Attachment #8651718 - Attachment is obsolete: true
Attachment #8651722 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/83f63500ab50
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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]:
Attachment #8651722 - Flags: approval-mozilla-beta?
Attachment #8651722 - Flags: approval-mozilla-aurora?
Georg, you marked it as wontfix for 42 and Philipp is asking for an uplift, do you agree?
Flags: needinfo?(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.
Flags: needinfo?(gfritzsche)
Comment on attachment 8651722 [details] [diff] [review]
telemetrytestfix.diff v3

OK, let's take it then.
Attachment #8651722 - Flags: approval-mozilla-beta?
Attachment #8651722 - Flags: approval-mozilla-beta+
Attachment #8651722 - Flags: approval-mozilla-aurora?
Attachment #8651722 - Flags: approval-mozilla-aurora+
Blocks: 1198364
You need to log in before you can comment on or make changes to this bug.