1393041 - Refactor and share the telemetry c++ utility functions in gtests

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

In bug 1343855 we found that we might need to verify/validate scalars from outside the TestScalars.cpp [1] file. This is a bit tricky, unless we share the helper functions defined in that file.

This bug is about:

- creating a new module with helper functions for testing scalar/histograms in C++ gtests;
- [bonus] add the test coverage requested in bug 1343855 comment 35.

[1] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TestScalars.cpp

Comment 1

[Alessio Placitelli [:Dexter]]

Fernando, would you be interested in working on this one?

Comment 2

[Fernando]

HI Alessio,

Sure! I'd like to work on this one :)

If I have any questions I'd post them here

Thank you very much!

Comment 3

[Alessio Placitelli [:Dexter]]

Thank you Fernando! Looking forward to seeing your contribution ;-)

Comment 4

[Fernando]

Hi Alessio,

Should I create a .h and a .cpp with the helper functions (and and the cpp to the moz.build file), or by "creating a new module" you mean something different?

Thank you very much!

Comment 5

[Fernando]

Hi again! :)

Is it possible that the test coverage requested in https://bugzilla.mozilla.org/show_bug.cgi?id=1343855#c35 is already implemented?

"::: toolkit/components/telemetry/Histograms.json
@@ +7514,5 @@
> +    "kind": "boolean",
> +    "keyed": true,
> +    "keys": [
> +      "testkey",
> +      "CommonKey"

For making sure indices are used correctly and in bounds, testing at least 3 keys might be good?
"

If I open Histograms.json, I can see three keys
"::: toolkit/components/telemetry/Histograms.json
@@ 7531 @@
 "keys": [
      "testkey",
      "CommonKey",
      "thirdKey"
    ],
"

And
"
::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +162,5 @@
> +              !expectedKeyData.isUndefined())
> +    << "Cannot find the expected key in the histogram data";
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, "not-allowed", &expectedKeyData) &&
> +              expectedKeyData.isUndefined())
> +    << "Unallowed keys must not be recorded in the histogram data";

We should also test for the sum being as expected in both?
"

It looks like this is already being tested in TestHistograms.cpp
"@@ 165 @@
  ASSERT_TRUE(!expectedKeyData.isUndefined())
    << "Cannot find the expected key in the histogram data";
  JS::RootedValue sum(cx.GetJSContext());
  GetProperty(cx.GetJSContext(), "sum", expectedKeyData,  &sum);
  uint32_t uSum = 0;
  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
  ASSERT_EQ(uSum, 0U) << "The histogram is not returning expected sum for 'testkey'";
"

Thank you very much!

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #4)
> Hi Alessio,
> 
> Should I create a .h and a .cpp with the helper functions (and and the cpp
> to the moz.build file), or by "creating a new module" you mean something
> different?
> 
> Thank you very much!

Yes, I mean that! Something like TelemetryTestHelpers.cpp/h

(In reply to Fernando from comment #5)
> Hi again! :)
> 
> Is it possible that the test coverage requested in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1343855#c35 is already
> implemented?

Yes, sorry, I linked to the wrong comment in the bug :) I meant bug 1343855 comment 39, "testing a separate code path that also ends up tracking into "telemetry.accumulate_unknown_histogram_keys"".

Comment 7

[Fernando]

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> (In reply to Fernando from comment #4)
> > Hi Alessio,
> > 
> > Should I create a .h and a .cpp with the helper functions (and and the cpp
> > to the moz.build file), or by "creating a new module" you mean something
> > different?
> > 
> > Thank you very much!
> 
> Yes, I mean that! Something like TelemetryTestHelpers.cpp/h
> 
> (In reply to Fernando from comment #5)
> > Hi again! :)
> > 
> > Is it possible that the test coverage requested in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1343855#c35 is already
> > implemented?
> 
> Yes, sorry, I linked to the wrong comment in the bug :) I meant bug 1343855
> comment 39, "testing a separate code path that also ends up tracking into
> "telemetry.accumulate_unknown_histogram_keys"".

Thank you very much! I'll keep working on this :)

Comment 8

[Fernando]

Hi Alessio,

I've moved the helper functions to a separate .h/.cpp files, and I can use them from TestHistograms.cpp, however, I'm not sure what I have to test there (or how to do it).
I see this in TestHistograms.cpp

"@@ 151 @@
 Telemetry::Accumulate("TELEMETRY_TEST_KEYED_KEYS", NS_LITERAL_CSTRING("not-allowed"), 1);
"

I think that ends up hitting this piece of code in TelemetryHistogram.cpp

" @@ 1887 @@
 // Check if we're allowed to record in the provided key, for this histogram.
  if (!gHistogramInfos[aID].allows_key(aKey)) {
    nsPrintfCString msg("%s - key '%s' not allowed for this keyed histogram",
                        gHistogramInfos[aID].name(),
                        aKey.get());
    LogToBrowserConsole(nsIScriptError::errorFlag, NS_ConvertUTF8toUTF16(msg));
    TelemetryScalar::Add(
      mozilla::Telemetry::ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
      NS_ConvertASCIItoUTF16(gHistogramInfos[aID].name()), 1);
"

And that's what I have to test, which I think that I could do it by adding something like this in TestHistograms.cpp
" @@ 187 @@
  TelemetryTestHelper::CheckUintScalar("telemetry.accumulate_unknown_histogram_keys", cx.GetJSContext(), scalarsSnapshot, 1);
"

Am I right?
I also don't know how to run the tests (I've tried  mach test toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js, but I don't think that this is the right thing to run)

Sorry, I know it's a lot!

Thank you very much!

Comment 9

[Fernando]

Hi Alessio,

Finally I was able to figure out how to run the tests, adding 
  TelemetryTestHelper::CheckUintScalar("telemetry.accumulate_unknown_histogram_keys", cx.GetJSContext(), scalarsSnapshot, 1);
To TestHistograms::TestKeyedKeysHistogram causes an exception on the line 
  ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test scalar must be reported.";

I'll keep investigating, but I'm a bit lost, if you could help me a bit that'd be great!

Thank you very much!

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #9)
> Hi Alessio,
> 
> Finally I was able to figure out how to run the tests, adding 
>  
> TelemetryTestHelper::CheckUintScalar("telemetry.
> accumulate_unknown_histogram_keys", cx.GetJSContext(), scalarsSnapshot, 1);
> To TestHistograms::TestKeyedKeysHistogram causes an exception on the line 
>   ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test
> scalar must be reported.";
> 
> I'll keep investigating, but I'm a bit lost, if you could help me a bit
> that'd be great!

It looks like you're definitely on the right path! Would you mind attaching your most recent patch so that I can have a look?

Comment 11

[Fernando]

Hi Alessio,
Attached you can find the patch with my most recent changes, the test that I'm not sure about (and it's currently wrong) is on TestHistograms.cpp:188

Thank you very much!

Comment 12

[Fernando]

Attachment: bug1393041_temporary.patch

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8924717 [details] [diff] [review]
bug1393041_temporary.patch

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

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +8,4 @@
>  #include "nsITelemetry.h"
>  #include "Telemetry.h"
>  #include "TelemetryFixture.h"
> +#include "TelemetryTestHelper.h"

Looks like this file is not included in this patch file :)

@@ +184,5 @@
>      << "Unallowed keys must not be recorded in the histogram data";
> +  JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  TelemetryTestHelper::GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
> +  //This line is wrong, but I'm not sure what to test here
> +  TelemetryTestHelper::CheckUintScalar("telemetry.accumulate_unknown_histogram_keys", cx.GetJSContext(), scalarsSnapshot, 2);

I think this is correct: this should count the number of times an accumulation with an unknown key was attempted within this test. The keys are defined here: http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/components/telemetry/Histograms.json#7582-7586

It looks like the "not-allowed" key accumulation was attempted twice, so we expect the value 2 to be in that scalar. Please add a comment stating so above the scalar check.

Comment 14

[Fernando]

Sorry, you are completely right, I forgot to add the new files to the patch.
I've attached the patch again (this time with the files there :)).

With the code as it is in the patch, it crashes on this line:
::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +191,4 @@
  TelemetryTestHelper::CheckUintScalar("not-allowed", cx.GetJSContext(),
                                       scalarsSnapshot, 2);

More exactly, on this assert

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelper.cpp
@@ +191,4 @@
  ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test scalar must be reported.";

If you can take a look that'd be great, anyway I'll keep digging to see if I can find out what's going on.

Thank you very much!

Comment 15

[Fernando]

Attachment: bug1393041_temporary.patch

Temporary patch with the new files added

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8925694 [details] [diff] [review]
bug1393041_temporary.patch

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

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +187,5 @@
> +  TelemetryTestHelper::GetScalarsSnapshot(false, cx.GetJSContext(),
> +                                          &scalarsSnapshot);
> +  // The "not-allowed" key accumulation was attemtped twice, so we expect
> +  // the the count to be 2
> +  TelemetryTestHelper::CheckUintScalar("not-allowed", cx.GetJSContext(),

"not-allowed" is not a scalar, this should be "telemetry.accumulate_unknown_histogram_keys" as it was in the previous patch :)

Comment 17

[Fernando]

Hi!

I've tried that, but it doesn't work (it fails on the same line)

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelper.cpp
@@ +191,4 @@
  ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test scalar must be reported.";

I'll keep trying to figure out how to test that.

Thank you very much!

Comment 18

[Alessio Placitelli [:Dexter]]

Comment on attachment 8925694 [details] [diff] [review]
bug1393041_temporary.patch

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

Good work Fernando, we're on the right track! I've highlighted the problem that was causing the assertion failure below. Since I was there, I left a few more comments ;) Keep up the great work! Please flag me for a review next time you attach an updated patch!

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelper.cpp
@@ +1,3 @@
> +#include "gtest/gtest.h"
> +
> +#include "TelemetryTestHelper.h"

Since this is a new file, let's follow the "#include" conventions from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices (C/C++ Practices section):

Includes are split into three blocks and are sorted alphabetically in each block:

    The main header: #include "Foo.h" in Foo.cpp
    Standard library includes: #include <map>
    Mozilla includes: #include "mozilla/dom/Element.h"

@@ +6,5 @@
> +#include "mozilla/Unused.h"
> +
> +using namespace mozilla;
> +
> +//Helper classes

nit: let's mention that these functions are only meant to be used in c++ gtests and are provided to simplify writing tests.

@@ +7,5 @@
> +
> +using namespace mozilla;
> +
> +//Helper classes
> +namespace TelemetryTestHelper {

nit: TelemetryTestHelpers (plural)?

@@ +121,5 @@
> +  Unused << JS_GetProperty(aCx, scalarObj, "parent", &parentScalars);
> +
> +  aResult.set(parentScalars);
> +}
> +} //namespace TelemetryTestHelper
\ No newline at end of file

nit: TelemetryTestHelpers

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelper.h
@@ +5,5 @@
> +
> +#include "js/TypeDecls.h"
> +
> +//Forward declarations
> +struct JSContext;

Isn't this already declared by http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/js/public/TypeDecls.h#25 ?

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +8,4 @@
>  #include "nsITelemetry.h"
>  #include "Telemetry.h"
>  #include "TelemetryFixture.h"
> +#include "TelemetryTestHelper.h"

Let's also move all the Histogram helper functions to TelemetryTestHelpers.h/cpp (e.g. GetAndClearHistogram)

@@ +160,4 @@
>    GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_KEYED_KEYS", snapshot, &histogram);
>  
>    // Get "testkey" property from histogram and check that it stores the correct
> +  // data.5000

nit: I think this 5000 should not be here ;)

@@ +183,5 @@
>    ASSERT_TRUE(expectedKeyData.isUndefined())
>      << "Unallowed keys must not be recorded in the histogram data";
> +
> +  JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  TelemetryTestHelper::GetScalarsSnapshot(false, cx.GetJSContext(),

Having a closer look at the code, I think I've found the problem :) The scalar we need to check is keyed (http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/toolkit/components/telemetry/Scalars.yaml#965), meaning that we need to pass "true" as the first parameter here and use CheckKeyedUintScalar below :)

Let me know if that helps!

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +33,4 @@
>  
>    // Check the recorded value.
>    JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  TelemetryTestHelper::GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);

Let's just add "using TelemetryTestHelper::GetScalarsSnapshot;" above and leave this unchanged. Save for the other functions below.

::: toolkit/components/telemetry/tests/gtest/moz.build
@@ +11,4 @@
>  ]
>  
>  UNIFIED_SOURCES = [
> +    'TelemetryTestHelper.cpp',

Let's rename the file to TelemetryTestHelpers.cpp (plural)

Comment 19

[Fernando]

Hi Alessio,

you are completely right! Yesterday I realized that I had to pass "true" because the scalar is keyed (I realized of this by comparing with TestScalars.cpp, where there are some similar tests).
After that the test still failed but it wasn't crashing. Doing some debugging I realized that I had to pass "TELEMETRY_TEST_KEYED_KEYS" as key in the TelemetryTestHelpers::CheckKeyedUintScalar (I was passing 'not-allowed', silly me!)
Now the test is working fine (and passing) :).

Attached you can find a patch, I've tackled all the things that you've commented in your previous post and fix them. There's something that I've done and I'm not sure if it's good or not (I haven't found anything against it in the code guidelines): I've used "using namespace TelemetryTestHelpers;" at the top of TestHistograms.cpp (I've done that because the namespace TelemetryTestHelpers is very long and I had to break every line where it was used because the total line length was more than 80 characters length). If you think that it's better to leave as it was before I can change it to not use "using namespace TelemetryTestHelpers;"

Thank you very much for your help!

Comment 20

[Fernando]

Attachment: bug1393041.patch

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8927044 [details] [diff] [review]
bug1393041.patch

Hey Ferdinando!

Thanks for following up so quickly, I'm glad that you had figured it out on your own. One detail for the future: when setting up a reviewer, make sure to pick the question mark "?" and write :dexter in the reviewer field. My name will pop-up and you will be able to select me. Don't set "+", as that means the patch was reviewed and it's ready for landing :)

Comment 22

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #21)
> Comment on attachment 8927044 [details] [diff] [review]
> bug1393041.patch
> 
> Hey Ferdinando!
> 
Ouch, sorry, I meant Fernando :D

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8927044 [details] [diff] [review]
bug1393041.patch

Fernando, it looks like the TelemetryTesHelpers.* files are missing from this patch. Can you please attach a new patch?

Comment 24

[Fernando]

Hi Alessio,

Sorry, as you said I forgot to add the TelemetryTestHelper files to the patch! And sorry about the reviewer thing, I wasn't sure what I had to do, thanks for telling me the right way to do it!

I've attached the full patch now :)

Thank you very much!

Comment 25

[Fernando]

Attachment: bug1393041.patch

Comment 26

[Alessio Placitelli [:Dexter]]

Comment on attachment 8927418 [details] [diff] [review]
bug1393041.patch

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

Thank you Fernando! This looks quite close to completion, there are a few minor changes to be done before we can ship it, but you're nearly there!

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelpers.cpp
@@ +1,1 @@
> +#include "TelemetryTestHelpers.h"

Can you add the copyright header at the top of this file? Exactly as done here: https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/toolkit/components/telemetry/tests/gtest/TelemetryFixture.h#1-3

@@ +3,5 @@
> +#include "gtest/gtest.h"
> +#include "mozilla/CycleCollectedJSContext.h"
> +#include "mozilla/Unused.h"
> +
> +

Remove one blank line from here :)

@@ +6,5 @@
> +
> +
> +using namespace mozilla;
> +
> +//Helper methods provided to simplify writing tests and meant to be used in C++ Gtests

nit: add a space between // and the first word. Please also add a trailing dot at the end of this sentence.

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelpers.h
@@ +1,1 @@
> +#ifndef TelemetryTestHelpers_h_

Can you add the copyright header at the top of this file? Exactly as done here: https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/toolkit/components/telemetry/tests/gtest/TelemetryFixture.h#1-3

@@ +3,5 @@
> +
> +#include "gtest/gtest.h"
> +
> +#include "js/TypeDecls.h"
> +#include "mozilla/OwningNonNull.h"

What is this header for?

@@ +48,5 @@
> +void
> +GetSnapshots(JSContext* cx, nsCOMPtr<nsITelemetry> mTelemetry,
> +             const char* name, JS::MutableHandleValue valueOut, bool is_keyed);
> +
> +} //namespace TelemetryTestHelpers

nit: add a space between // and namespace

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +19,4 @@
>    AutoJSContextWithGlobal cx(mCleanGlobal);
>  
>    GetAndClearHistogram(cx.GetJSContext(), mTelemetry, NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),
> +                                             false);

Can you drop the changes to this line? (i.e. preserve the old indentation by aligning it to |cx.GetJSContext()|.

@@ +103,4 @@
>    GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_KEYED_KEYS", snapshot, &histogram);
>  
>    // Get "testkey" property from histogram and check that it stores the correct
> +  // data

Can you drop the changes to this line? (i.e. add back the trailing dot)

@@ +114,4 @@
>    JS::ToUint32(cx.GetJSContext(), sum, &uSum);
>    ASSERT_EQ(uSum, 0U) << "The histogram is not returning expected sum for 'testkey'";
>  
> +

No need for this blank line :)

@@ +239,4 @@
>    uint32_t uOtherValue = 0;
>    JS::ToUint32(cx.GetJSContext(), otherValue, &uOtherValue);
>    ASSERT_EQ(uOtherValue, kOtherSampleExpectedValue) << "The other-sample histogram is not returning expected value";
> +}
\ No newline at end of file

Can you please add the blank line at the end of this file back?

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +33,4 @@
>  
>    // Check the recorded value.
>    JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  TelemetryTestHelpers::GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);

Instead of adding |TelemetryTestHelpers::| to all the calls like this one, what about just adding |using namespace TelemetryTestHelpers;| at the top of this file as you did for TestHistograms.cpp?

Comment 27

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #24)
> Hi Alessio,
> 
> Sorry, as you said I forgot to add the TelemetryTestHelper files to the
> patch! And sorry about the reviewer thing, I wasn't sure what I had to do,
> thanks for telling me the right way to do it!

No problem at all, don't worry! That's part of the learning process :)

Comment 28

[Fernando]

Hi Alessio,

Thanks very much for reviewing my changes so fast! :)
Following your comments I've updated the patch (I've also added a blank at the end of TestScalars.cpp and broken some lines that were longer than 80 characters).

I hope everything is fine now, sorry for the previous patch, thanks to your review I realized that the patch was a bit messy!

Thank you very much!

Comment 29

[Fernando]

Attachment: bug1393041.patch

Comment 30

[Alessio Placitelli [:Dexter]]

Comment on attachment 8928007 [details] [diff] [review]
bug1393041.patch

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

Hi Fernando, you did a great work on this bug so far! Sorry for being too nitpicky :) There's a couple of changes left to do and then we're good to go :)

No need to apologize for the previous patch, don't worry, you're doing good!

Let me know if anything is unclear.

::: toolkit/components/telemetry/tests/gtest/TelemetryTestHelpers.h
@@ +4,5 @@
> +
> +#ifndef TelemetryTestHelpers_h_
> +#define TelemetryTestHelpers_h_
> +
> +#include "gtest/gtest.h"

We don't need this include here.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +125,5 @@
>    GetProperty(cx.GetJSContext(), "not-allowed", histogram,  &expectedKeyData);
>    ASSERT_TRUE(expectedKeyData.isUndefined())
>      << "Unallowed keys must not be recorded in the histogram data";
> +
> +

nit: we just need one blank line here :)

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +35,4 @@
>    // Check the recorded value.
>    JS::RootedValue scalarsSnapshot(cx.GetJSContext());
>    GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
> +  CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(),

I know this line (and the ones below) are longer than the expected/allowed length. However we don't need to attack this in this bug: it will only complicate tracking down problems by analysing the commit history/blame in case some problem arises :) Let's just drop all the line changes below this point

Comment 31

[Fernando]

Hi Alessio,

I've put the lines again as they were, and remove the unused header and the extra blank line.
I hope that I got everything right this time! Thanks for all your reviews :)

Thanks!

Comment 32

[Fernando]

Attachment: bug1393041.patch

Comment 33

[Alessio Placitelli [:Dexter]]

Comment on attachment 8928262 [details] [diff] [review]
bug1393041.patch

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

This looks great now, thanks Fernando! Let me push this to our try servers.

Comment 34

[Alessio Placitelli [:Dexter]]

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

Comment 35

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94111a55e8c3dfc3cb526650c7a18ea9e7f5d85e
Bug 1393041 - Refactor and share the telemetry c++ utility functions in gtests. r=dexter

Comment 36

[Alessio Placitelli [:Dexter]]

Hey Fernando, I hope you're having fun with these bugs :) Would you be interested in taking over another bug? Maybe a slightly more involved one if you feel like it?

Comment 37

[Fernando]

Hi Alessio,

I do have fun! It'd be great to work on another bug!! :)

Thank you very much!

Comment 38

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/94111a55e8c3

Comment 39

[Gerry Chang [:gchang]]

Won't fix for 57/58. Let it ride the train.