Add test coverage for applying recorded scalar actions

RESOLVED FIXED in Firefox 63

Status

()

P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: janerik, Assigned: petru.gurita1, Mentored)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [good second bug][lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
In bug 1470897 we changed the code to catch invalid data, log a warning and continue processing recorded operations.
We should add test cases to cover these cases, e.g. recording scalar operations on the wrong type of scalar, apply them and observe that the scalar didn't change (and that the process didn't crash).
(Reporter)

Updated

9 months ago
Whiteboard: [good second bug][lang=c++]
(Reporter)

Comment 1

9 months ago
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.
- Create a new test in toolkit/components/telemetry/tests/gtest/TestScalars.cpp by copying one of the existing ones and give it a reasonable name
- Make sure to call |mTelemetry->ClearScalars()| at the beginning of the test
- Set initial values for some test scalars of different types (uint, string, boolean). You'll find the names in toolkit/components/telemetry/Scalars.yaml in the telemetry.test section. You can do so by calling |Telemetry::ScalarSet|.
- Call |TelemetryScalar::DeserializationStarted()| to start the recording mode
- Record different invalid operations for the previous select scalars, e.g. try |Telemetry::ScalarAdd| on the string scalar.
- Call |TelemetryScalar::ApplyPendingOperations|
- Check that the scalars still contain the initial values, use |CheckUintScalar|, |CheckBoolScalar|, |CheckStringScalar|.
- Repeat the above steps to add another test case, but choose keyed scalars this time (e.g. |telemetry.test.keyed_unsigned_int|).
- 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 TelemetryTestFixture*`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch (including an automated test, if applicable) 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.
(Reporter)

Updated

9 months ago
Depends on: 1470897
(Assignee)

Comment 2

9 months ago
Hello, I want to give it a shot. I will start solving it tomorrow morning.
(Reporter)

Comment 3

9 months ago
Wonderful. I'll assign you right away. Let me know if you need any help.
(Reporter)

Updated

9 months ago
Assignee: nobody → petru.gurita1
Priority: -- → P1
(Assignee)

Comment 4

9 months ago
Hello, after running './mach gtest TelemetryTestFixture*' I got this error:
19:00.16 /home/petru/firefox/mozilla-central/dom/base/nsMappedAttributes.cpp:16:10: fatal error: 'mozilla/MappedDeclarations.h' file not found
19:00.16 #include "mozilla/MappedDeclarations.h"
19:00.16          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
19:02.93 1 error generated.

Also, is there any other way to see if my test function works accordingly? './mach gtest TelemetryTestFixture*' is pretty slow.(19 mins +)
(Reporter)

Comment 5

9 months ago
The error looks weird. Do you have a fresh and up-to-date checkout of the Firefox code base?

For the first initial compile it will take a long time, expect 30-50 minutes depending on your system.
Subsequent executions of './mach gtest TelemetryTestFixture*' should be faster, even if you change the tests it should compile and run in a minute or two.
(Assignee)

Comment 6

9 months ago
Yes I do have  a fresh and up-to-date checkout of the Firefox code base.
Do you think this might happen due to my function? I'll run the tests again, this time without my function and see what happens.
(Reporter)

Comment 7

9 months ago
The compilation error? It shouldn't. That happens in a completely unrelated file. And it is from a change from 2018-06-22, so any newer checkout should be just fine. 
Does your checkout have the file "layout/style/MappedDeclarations.h"? If not, you need to update as something is wrong.
(Assignee)

Comment 8

9 months ago
If I run `hg pull -u` the result is:
pulling from https://hg.mozilla.org/mozilla-central
searching for changes
no changes found

However, I keep getting this error:
0:13.37 /home/petru/firefox/mozilla-central/dom/base/nsMappedAttributes.cpp:16:10: fatal error: 'mozilla/MappedDeclarations.h' file not found
0:13.37 #include "mozilla/MappedDeclarations.h"

I do have the file "layout/style/MappedDeclarations.h".
Comment hidden (mozreview-request)
(Assignee)

Comment 10

9 months ago
Sorry, I had a problem with clobber and that's why I couldn't run the tests. Now everything works fine.

I finally added a test. I can add more test cases if needed. Right now I'm testing the string and boolean scalar types  with ScalarAdd.
(Reporter)

Comment 11

9 months ago
I'm sorry you had all this trouble. I'm glad you were able to figure it out. I'll review the patch later today.
(Reporter)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8990578 [details]
Bug 1473520 - Add test coverage for applying recorded scalar actions.

https://reviewboard.mozilla.org/r/255644/#review262456

This is a very good start.
Two small things left:

1. Could you please add one more case, handling TELEMETRY_UNSIGNED_INT_KIND and trying to set it to a string?
2. Could you add a second test function to do similar tests for the keyed scalars `keyed_unsigned_int` and `keyed_boolean_kind`?
Attachment #8990578 - Flags: review?(jrediger)
(Reporter)

Comment 13

8 months ago
As per your private messages:

Re hg commits:
You need to create a new commit for this patch, not re-editing an old commit (which is what `hg commit --amend` does).
See http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/workflows.html for documentation around the workflow.
You can switch heads using `hg head commit-id`.
You create a new commit using `hg commit`
You can then switch back and forth between your different heads. Mercurial will not lose already committed data.

Re crashes after extending the test:
It sounds like exactly what these tests should cover. I fixed the code in bug 1470897 and just ran a very small test locally to verify. I think you might still be working off of an old checkout and thus don't have the required patches in there.
Update your local checkout using `hg pull`, then switch to the latest commit using `hg up central`. After that you most likely require a fresh build (`mach build`, if that fails do a `mach clobber` first, again this might take some time)

Do these things help? If anything is still unclear or you still have compile or test failures please reach out. I'm available on IRC during workdays in UTC+2 (mostly 9:00-18:00 UTC+). If I don't answer there, leaving a comment here is just as fine.
(Reporter)

Comment 14

8 months ago
Re keyed scalars with string values:
We explicitly don't support these, see https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/docs/telemetry/collection/scalars.html#keyed-scalars
Therefore you can test it with:

    Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_UNSIGNED_INT, NS_LITERAL_STRING("key1"), true);

Once you've done that, please submit the patch. I think it should be mostly complete now.
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8990578 - Attachment is obsolete: true
(Reporter)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8992442 [details]
Bug 1473520 - Add test coverage for applying recorded scalar actions.

https://reviewboard.mozilla.org/r/257308/#review264328

I just have a couple more minor things. 
I also kicked off a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c14cd92b549f491439e686f51e31f3b9919f9ef

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:327
(Diff revision 1)
> +
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_STRING_KIND, NS_LITERAL_STRING(EXPECTED_STRING));
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true);

It would still be nice to have the uint case covered as well.

Start by adding this here:

```
const uint32_t expectedValue = 1172015;
Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, expectedValue);
```

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:332
(Diff revision 1)
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true);
> +
> +  TelemetryScalar::DeserializationStarted();
> +
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_STRING_KIND, 1447);
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, 1447);

Now add:

``` Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, true);
```

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:338
(Diff revision 1)
> +  TelemetryScalar::ApplyPendingOperations();
> +
> +  JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
> +  CheckStringScalar("telemetry.test.string_kind", cx.GetJSContext(), scalarsSnapshot, EXPECTED_STRING);
> +  CheckBoolScalar("telemetry.test.boolean_kind", cx.GetJSContext(), scalarsSnapshot, true);

Finally check the value:

```
CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, expectedValue);
```

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:342
(Diff revision 1)
> +  CheckStringScalar("telemetry.test.string_kind", cx.GetJSContext(), scalarsSnapshot, EXPECTED_STRING);
> +  CheckBoolScalar("telemetry.test.boolean_kind", cx.GetJSContext(), scalarsSnapshot, true);
> +}
> +
> +TEST_F(TelemetryTestFixture, WrongKeyedScalarOperator) {
> +

Don't need this newline here.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:348
(Diff revision 1)
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +

One newline too many.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:371
(Diff revision 1)
> +  GetScalarsSnapshot(true, cx.GetJSContext(), &scalarsSnapshot);
> +  CheckKeyedUintScalar("telemetry.test.keyed_unsigned_int", "key1",
> +                       cx.GetJSContext(), scalarsSnapshot, kExpectedUint);
> +  CheckKeyedBoolScalar("telemetry.test.keyed_boolean_kind", "key2",
> +                       cx.GetJSContext(), scalarsSnapshot, true);
> +

Don't need this newline here.
Attachment #8992442 - Flags: review?(jrediger)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

8 months ago
I think it's done now. Do you have any other tasks for me ?
(Reporter)

Comment 19

8 months ago
mozreview-review
Comment on attachment 8992442 [details]
Bug 1473520 - Add test coverage for applying recorded scalar actions.

https://reviewboard.mozilla.org/r/257308/#review264590
Attachment #8992442 - Flags: review+
(Reporter)

Comment 20

8 months ago
mozreview-review
Comment on attachment 8992750 [details]
Bug 1473520 - Add test coverage for applying recorded scalar actions.

https://reviewboard.mozilla.org/r/257606/#review264592
Attachment #8992750 - Flags: review?(jrediger) → review+

Comment 21

8 months ago
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b47db3a7a809
Add test coverage for applying recorded scalar actions. r=janerik
https://hg.mozilla.org/integration/autoland/rev/54665bb153f1
Add test coverage for applying recorded scalar actions. r=janerik
(Reporter)

Comment 22

8 months ago
I just pushed the changes, it will now take some time to actually land upstream (but should be done within the day).
I'll see if I can find some other good bugs for you.

Comment 23

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b47db3a7a809
https://hg.mozilla.org/mozilla-central/rev/54665bb153f1
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.