1375481 - Remove unnecessary histogram's clearing in test_TelemetrySend.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Kate Ustiuzhanina]

In the test_TelemetrySend.js(test_discardBigPings) there is unnecessary clering of two histograms after clearing them in the loop. 
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetrySend.js?q=test_TelemetrySend.js&redirect_type=direct#329

Additionaly, in tho whole file histogram's clearing is not unified: sometimes they clear in the loop, sometimes not. (The same for other files: test_TelemetrySendOldPings.js, test_PingAPI.js)

Comment 1

[Georg Fritzsche [:gfritzsche]]

I think for the purpose of this specific bug, removing the unnecessary `clear()`s is enough.

Comment 2

[Georg Fritzsche [:gfritzsche]]

This will involve:
* removing the unnecessary lines
* running the test to confirm things are working properly: ./mach test toolkit/components/telemetry/tests/unit/test_TelemetrySend.jsm

Comment 3

[Nils M.]

I would take this bug if it's ok. TO get ir right: it's just to remove the lines in test_TelemetrySend.js?

Another question regarding testing. Running the test with 

>./mach test toolkit/components/telemetry/tests/unit/test_TelemetrySend.js

results in an error:

> 0:00.18 LOG: MainThread WARNING MOZ_NODE_PATH environment variable not set. Tests requiring http/2 will fail.
> Error running mach:
> 
>    ['test', 'toolkit/components/telemetry/tests/unit/test_TelemetrySend.js']
> 
> The error occurred in code that was called by the mach command. This is either
> a bug in the called code itself or in the way that mach is calling it.
> 
> You should consider filing a bug for this issue.

. Applying the workaround

>./mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetrySend.jsm

as it was suggested in bug 1375482 comment 3 seems to work. Is this ok here, too?

Comment 4

[Chris H-C :chutten]

Yup! That's just the test harness complaining that you don't have nodejs configured, which you shouldn't need for these tests.

Comment 5

[Nils M.]

I've did the changes locally and could submit the patch (hopefully via MozReview using hg push review if all is set up right). Should I wait until the bug is assigned to me? If so, who would assign it to me?

Comment 6

[Georg Fritzsche [:gfritzsche]]

I assigned it ago, so go ahead!

Comment 7

[Nils M.]

Attachment: Bug 1375481 - Removed unnecessary clearings of histograms

Review commit: https://reviewboard.mozilla.org/r/152502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152502/

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8881302 [details]
Bug 1375481 - Removed unnecessary clearings of histograms

https://reviewboard.mozilla.org/r/152502/#review157918

This looks good, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/299496ea4cac
Removed unnecessary clearings of histograms r=gfritzsche

Comment 10

[Nils M.]

Thanks a lot for the review and pushing the path!

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/299496ea4cac