Closed
Bug 1443505
Opened 7 years ago
Closed 3 years ago
Test Abnormal keys for Keyed Scalars
Categories
(Toolkit :: Telemetry, defect, P4)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
91 Branch
Tracking | Status | |
---|---|---|
firefox91 | --- | fixed |
People
(Reporter: chutten, Assigned: mstarzycki)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [lang=c++])
Attachments
(2 files, 3 obsolete files)
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.
status-firefox60:
--- → affected
Comment 1•7 years ago
|
||
HI Chris,
I will start with this Bugs. Can you assign it to me?
Reporter | ||
Comment 2•7 years ago
|
||
Certainly. Please let me know if you have any questions!
Assignee: nobody → phsangeetha1
Status: NEW → ASSIGNED
Reporter | ||
Updated•7 years ago
|
Priority: -- → P4
Reporter | ||
Comment 4•7 years ago
|
||
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)
Assignee: phsangeetha1 → is2ei.horie
Flags: needinfo?(chutten)
Comment 5•6 years ago
|
||
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".
Flags: needinfo?(chutten)
Reporter | ||
Comment 6•6 years ago
|
||
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
Flags: needinfo?(chutten)
Comment 7•6 years ago
|
||
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
Flags: needinfo?(chutten)
Reporter | ||
Comment 8•6 years ago
|
||
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?
Flags: needinfo?(chutten)
Comment 9•6 years ago
|
||
Thanks a lot for your help, Chris!
I created a patch. Could you review it, please?
Attachment #8983987 -
Attachment is obsolete: true
Attachment #8984734 -
Flags: review?(chutten)
Reporter | ||
Comment 10•6 years ago
|
||
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
Attachment #8984734 -
Flags: review?(chutten) → review+
Comment 11•6 years ago
|
||
Thanks for your comment!
I updated the patch. Could you check it, please?
Attachment #8984734 -
Attachment is obsolete: true
Attachment #8985824 -
Flags: review?(chutten)
Reporter | ||
Comment 12•6 years ago
|
||
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.
Attachment #8985824 -
Flags: review?(chutten)
Reporter | ||
Comment 13•6 years ago
|
||
Oh good, they're on the correct lines. Just these small comment changes and we'll get this checked in!
Comment 14•6 years ago
|
||
Thanks for your comment!
I updated the patch. Could you check it, please?
Attachment #8985824 -
Attachment is obsolete: true
Attachment #8986455 -
Flags: review?(chutten)
Reporter | ||
Comment 15•6 years ago
|
||
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?
Attachment #8986455 -
Flags: review?(chutten) → review+
Reporter | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 16•6 years ago
|
||
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4c085cf228
Test Abnormal keys for Keyed Scalars r=chutten
Keywords: checkin-needed
Comment 17•6 years ago
|
||
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e0c1917083
Backed out changeset 7f4c085cf228 for failing Gtests on Assertion failure TelemetryScalar.cpp:773
Comment 18•6 years ago
|
||
(In reply to Pulsebot from comment #17)
> Backout by aiakab@mozilla.com:
> 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.
Flags: needinfo?(chutten)
Reporter | ||
Comment 19•6 years ago
|
||
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.
Flags: needinfo?(chutten)
Updated•5 years ago
|
Keywords: good-first-bug
Whiteboard: [good first bug] [lang=c++] → [lang=c++]
Comment 20•4 years ago
|
||
This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.
Assignee: is2ei.horie → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 21•3 years ago
|
||
I would like to pick this one up if there is no continuation from previous parties.
Assignee | ||
Comment 22•3 years ago
|
||
Updated•3 years ago
|
Assignee: nobody → mstarzycki
Status: NEW → ASSIGNED
Comment 23•3 years ago
|
||
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee5bcf34157d
tests conditionally added for release only r=chutten
Comment 24•3 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox91:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•