Closed Bug 1389992 Opened 2 years ago Closed 2 years ago

test_TelemetrySession.js is going to permafail when the Gecko version number is bumped to 58 on 2017-09-18

Categories

(Toolkit :: Telemetry, defect, P2, critical)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 - verified

People

(Reporter: RyanVM, Assigned: RyanVM, Mentored)

Details

(Whiteboard: [measurement:client][lang=js])

Attachments

(1 file)

[Tracking Requested - why for this release]: Permafailing tests on the next merge day after the Gecko version bump to 58.

I ran a simulation on Try of the next Gecko version number bump (https://hg.mozilla.org/try/rev/2032ed1ce47f754b7f6399168bb29cf93cddde09) and I hit a failure in test_TelemetrySession.js that needs investigation. Can you please take a look, Alessio?

https://treeherder.mozilla.org/logviewer.html#?job_id=122804707&repo=try

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySession.js | test_sendShutdownPing - [test_sendShutdownPing : 1397] The OS shutdown scalar must be set to true. - "undefined" == true
Flags: needinfo?(alessio.placitelli)
Priority: -- → P1
Whiteboard: [measurement:client]
Thanks for filing this, Ryan!

The failure is due to the |os_shutting_down| scalar expiring on 58 [1]. I think we should let it expire and remove the related code: this scalar has been used to validate the shutdown pingsender and it's not being used for anything else.

@Georg, any thought on that? Should we keep this or go on and remove it?

[1] - http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/toolkit/components/telemetry/Scalars.yaml#700
Flags: needinfo?(alessio.placitelli) → needinfo?(gfritzsche)
Removal sounds fine to me.
I think there are two parts here:
- fixing the beta test failure in this bug
- removing the code, which can be a separate (mentored?) bug
Flags: needinfo?(gfritzsche)
It's a very monday monday: i see now that this will happen on Nightly going to 58, so yes, lets just remove things.
This bug is about removing the "os_shutting_down" scalar from the Telemetry code [1] and the related test coverage [2]. In order to make sure that everything is working after the changes, xpcshell tests can be run using the following command:

> ./mach test toolkit/components/telemetry

[1] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySend.jsm#799
[2] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#1379
Mentor: alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client][lang=js]
Priority: P1 → P2
Is it OK to remove it now from 57 or did you want to wait until after the version is bumped to 58?
Flags: needinfo?(gfritzsche)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Is it OK to remove it now from 57 or did you want to wait until after the
> version is bumped to 58?

As per comment 3, it's ok to remove it from 57.
Flags: needinfo?(gfritzsche)
Try looks happy.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4ce0eb02155636975275b753b26bebc5b6e23b8
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #8900296 - Flags: review?(alessio.placitelli)
Comment on attachment 8900296 [details] [diff] [review]
remove the os_shutting_down scalar and related code

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

Thanks for the help Ryan, this looks good.
Attachment #8900296 - Flags: review?(alessio.placitelli) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8350c7b06d
Remove the os_shutting_down scalar and related code. r=dexter
https://hg.mozilla.org/mozilla-central/rev/de8350c7b06d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Status: RESOLVED → VERIFIED
This is fixed, no need to track it.
You need to log in before you can comment on or make changes to this bug.