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

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

[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

Comment 1

[Alessio Placitelli [:Dexter]]

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

Comment 2

[Georg Fritzsche [: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

Comment 3

[Georg Fritzsche [: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.

Comment 4

[Alessio Placitelli [:Dexter]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is it OK to remove it now from 57 or did you want to wait until after the version is bumped to 58?

Comment 6

[Alessio Placitelli [:Dexter]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Attachment: remove the os_shutting_down scalar and related code

Try looks happy.

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

Comment 8

[Alessio Placitelli [:Dexter]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8350c7b06d
Remove the os_shutting_down scalar and related code. r=dexter

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de8350c7b06d

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is fixed, no need to track it.