1443505 - Test Abnormal keys for Keyed Scalars

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Chris H-C :chutten]

We have c++ tests for Scalars in toolkit/components/telemetry/tests/gtest/TestScalars.cpp

But we don't test what happens if we try to use a key that is too long (longer than 70 characters) or empty. We also don't test what happens if we try to use too many (more than 100) keys on the same scalar.

This task is to write a new test that tries to accumulate to a test keyed scalar with those unusual cases (too long, empty, too many) to ensure that we handle them properly. The test KeyedScalarUnsigned is a good place to start.

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. I have written out some hints above.
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build` and test your change with `mach gtest Telemetry*`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 1

[sangeetha PH]

HI Chris,
        I will start with this Bugs. Can you assign it to me?

Comment 2

[Chris H-C :chutten]

Certainly. Please let me know if you have any questions!

Comment 3

[Issei Horie [:is2ei]]

Hi Chris,
Could I work on this issue?

Comment 4

[Chris H-C :chutten]

After two months of inactivity, I think this is safe to reassign, yes. (Geez, I need to go through my mentored bugs and look for stalled ones)

Comment 5

[Issei Horie [:is2ei]]

Hi Chris,

I have a question. Where can I find "TELEMETRY_TEST_STRING_KIND"?
I would like to define new ScalarID for the test in "toolkit/components/telemetry/tests/gtest/TestScalars.cpp".

Comment 6

[Chris H-C :chutten]

TELEMETRY_TEST_STRING_KIND is the scalar ID for telemetry.test.string_kind[1]. It has to look LIKE_THIS to fit the C++ constant identifier style (no dots, and all uppercase).

To add a new Scalar, you can add it in the telemetry.test area of Scalars.yaml. telemetry.test scalars are omitted from being sent just in case a test bot gets access to the internet :)

Does that help?

[1]: https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/components/telemetry/Scalars.yaml#1750

Comment 7

[Issei Horie [:is2ei]]

Attachment: screenshot crashed message

Hi Chris,

Thanks for the info! It helps a lot :-)

Could I ask you another question?
When I tried to test too long key (more than 70 chars), the message window appears and it tells me Firefox crashed.

How can I approach to this issue? I have 2 idea.

1. Add test to make sure it will crash if we use too long key.
2. Change the Scalar related function to avoid crash.

I pasted my change here. 
https://pastebin.mozilla.org/9087175

Comment 8

[Chris H-C :chutten]

I wonder if we have an ASSERT to crash if we get too-long keys in the C++ API... No, we return an error value and proceed: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryScalar.cpp#859

Did the test output have a callstack that told you where in the code it crashed?

Or maybe CheckKeyedUintScalar assert-failed... Yeah, look, it'll assert (crash) if the key isn't present: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TelemetryTestHelpers.cpp#57

You may have to check the snapshot directly to ensure the key isn't present 

...oh, and here's a possible misunderstanding. If you pass a too-long key, TelemetryScalar won't shorten it. It'll just do nothing at all with it. You have to shorten it yourself.

Does any of that help?

Comment 9

[Issei Horie [:is2ei]]

Attachment: bug-1443505.patch

Thanks a lot for your help, Chris!

I created a patch. Could you review it, please?

Comment 10

[Chris H-C :chutten]

Comment on attachment 8984734 [details] [diff] [review]
bug-1443505.patch

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

Just some small comment fixes needed, and we'll get this in!

Oh, and be sure to put "r=chutten" at the end of your commit message's first line so that if anything goes wrong they know to ask me about it :)

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ -316,3 @@
>    // which all end up in the same place.
>    CheckKeyedUintScalar(kScalarName, kLongestEvent, cx.GetJSContext(), scalarsSnapshot, 2);
>  }

This comment isn't correct. It should be rewritten to describe what we're testing, or it can be omitted.

@@ -316,3 @@
>    // which all end up in the same place.
>    CheckKeyedUintScalar(kScalarName, kLongestEvent, cx.GetJSContext(), scalarsSnapshot, 2);
>  }

And here

@@ -316,3 @@
>    // which all end up in the same place.
>    CheckKeyedUintScalar(kScalarName, kLongestEvent, cx.GetJSContext(), scalarsSnapshot, 2);
>  }

Same here

Comment 11

[Issei Horie [:is2ei]]

Attachment: bug-1443505.patch

Thanks for your comment!
I updated the patch. Could you check it, please?

Comment 12

[Chris H-C :chutten]

Comment on attachment 8985824 [details] [diff] [review]
bug-1443505.patch

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

Oh, goodness, Splinter (the code review thing) became confused. I'm sorry, it was the " // Set the test scalar to a known value." comments that weren't correct any longer, not "which all end up in the same place". I'm not sure how it decided to move all of my comments up that high in the diff and keep all three of them...

Let's see if it will let me highlight the proper lines this time.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +322,5 @@
> +
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +  // Set the test scalar to a known value.

This one.

@@ +344,5 @@
> +
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +  // Set the test scalar to a known value.

And this one.

@@ +366,5 @@
> +
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +  // Set the test scalar to a known value.

And this one.

Comment 13

[Chris H-C :chutten]

Oh good, they're on the correct lines. Just these small comment changes and we'll get this checked in!

Comment 14

[Issei Horie [:is2ei]]

Attachment: bug-1443505.patch

Thanks for your comment!
I updated the patch. Could you check it, please?

Comment 15

[Chris H-C :chutten]

Comment on attachment 8986455 [details] [diff] [review]
bug-1443505.patch

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

Looks great, thanks! I'll mark this for checkin. (It's the addition of some test coverage, so it ought to be safe for inclusion at this point in 62)

Would you like some help finding something else to work on? If so, what are you interested in?

Or did you already have an idea of what you'd like to do next?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4c085cf228
Test Abnormal keys for Keyed Scalars r=chutten

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e0c1917083
Backed out changeset 7f4c085cf228 for failing Gtests on Assertion failure TelemetryScalar.cpp:773

Comment 18

[Issei Horie [:is2ei]]

(In reply to Pulsebot from comment #17)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e0c1917083
> Backed out changeset 7f4c085cf228 for failing Gtests on Assertion failure
> TelemetryScalar.cpp:773

Hi :chutten

This means my patch does not work, right?
Sorry for bothering you. I will check my patch.

Comment 19

[Chris H-C :chutten]

Ah, yes... it specifically "doesn't work" in the way we want it to not work by complaining the key was too long: https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/telemetry/TelemetryScalar.cpp#773

MOZ_ASSERT only asserts on debug builds, so we'll need to guard your test to only run on Release builds. See the other tests that have #ifndef DEBUG  protecting them for ideas of how to format them.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 21

[Cerkiewny]

I would like to pick this one up if there is no continuation from previous parties.

Comment 22

[Cerkiewny]

Attachment: Bug 1443505 - tests conditionally added for release only r?chutten

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee5bcf34157d
tests conditionally added for release only r=chutten

Comment 24

[Narcis Beleuzu [:NarcisB]]

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