1471144 - Add gtest coverage for CombinedStacks

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

While CombinedStacks [1] is indirectly covered by the xpcshell test coverage for KeyedStackCapturer [2], it might be a good idea to add gtest unit testing for it.

We should at least test adding and removing stacks.

[1] - https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/telemetry/CombinedStacks.cpp
[2] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryCaptureStack.js#61

Comment 1

[Alessio Placitelli [:Dexter]]

To do this, we should:

- add a new test file (e.g. "TestCombinedStacks.cpp") in https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest
- add a test case |TEST_F(...)| to the file, as done in https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/toolkit/components/telemetry/tests/gtest/TestCounters.cpp#16
- add the new file to the list in moz.build here: https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/toolkit/components/telemetry/tests/gtest/moz.build#17

To make sure the tests are working, the following command can be used:

> ./mach gtest Telemetry*

Comment 2

[Petru Gurita]

Attachment: Bug 1471144 - Add gtest coverage for 'CombinedStacks' .

Added testcase for 'AddStack' method used by 'CombinedStacks'.

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

Comment 3

[Petru Gurita]

I am not sure if I am on the right track.
1. I don't know if I tested the Add operation as intended/ properly.
2. I don't know which method to call in order to test the remove stack method.
3. Should I use an `AutoJSContextWithGlobal` object for different calls (I've seen this in most of the tests from gtest/)? If yes, how do I find which one?
Thanks!

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8990609 [details]
Bug 1471144 - Add gtest coverage for 'CombinedStacks' .

https://reviewboard.mozilla.org/r/255666/#review262422

This looks like a good start, thank you for your contribution! While the current test would work, I think we should test the behaviour of the `AddStack` function a bit deeper, as explained below.

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:1
(Diff revision 1)
> +#include "CombinedStacks.h"

We should add the copyright header before the include, as done [here](https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp#2-4)

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:3
(Diff revision 1)
> +#include "CombinedStacks.h"
> +
> +namespace mozilla {

Let's not add tests to these namespaces: there's no need to

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:8
(Diff revision 1)
> +namespace mozilla {
> +  namespace Telemetry {
> +
> +    TEST_F(TelemetryTestFixture, CombinedStacksAdd) {
> +
> +      const size_t kMaxChromeStacksKept = 10;

nit: let's call this `kMaxStacksKept`

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:9
(Diff revision 1)
> +  namespace Telemetry {
> +
> +    TEST_F(TelemetryTestFixture, CombinedStacksAdd) {
> +
> +      const size_t kMaxChromeStacksKept = 10;
> +      CombinedStacks cStack(kMaxChromeStacksKept);

nit: let's call this simply `stacks`

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:11
(Diff revision 1)
> +    TEST_F(TelemetryTestFixture, CombinedStacksAdd) {
> +
> +      const size_t kMaxChromeStacksKept = 10;
> +      CombinedStacks cStack(kMaxChromeStacksKept);
> +
> +      for (size_t i = 0; i < kMaxChromeStacksKept; ++i) {

Since we're making the `CombinedStacks` instance hold 10 elements, we should try to push more than that to it: it's a circular queue, so it should overwrite the oldest elements it contains. Let's make sure of that.

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:12
(Diff revision 1)
> +
> +      const size_t kMaxChromeStacksKept = 10;
> +      CombinedStacks cStack(kMaxChromeStacksKept);
> +
> +      for (size_t i = 0; i < kMaxChromeStacksKept; ++i) {
> +        cStack.AddStack(ProcessedStack());

To verify that we're overwriting the right elements when adding more than 10, we could add some data to `ProcessedStack`. We can do something like that (see the interface [here](https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/ProcessedStack.h#26)):

```
ProcessedStack s;
ProcessedStack::Module m = { ... some data here ...};
ProcessedStack::Frame f = {...data..};
s.AddModule(m);
s.AddFrame(f);
stacks.AddStack(s);
```

Of course the data from `m` and `f` would need to change at each iteration.

::: toolkit/components/telemetry/tests/gtest/TestCombinedStacks.cpp:15
(Diff revision 1)
> +
> +      for (size_t i = 0; i < kMaxChromeStacksKept; ++i) {
> +        cStack.AddStack(ProcessedStack());
> +      }
> +
> +      const size_t actualValue = cStack.GetStackCount();

We can simply use `stacks.GetStackCount()` and `kMaxStacksKept` in the `ASSERT_EQ` below, without assigning them to constants that are just once.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Petru Gurita from comment #3)
> I am not sure if I am on the right track.
> 1. I don't know if I tested the Add operation as intended/ properly.

You started good! I left a few notes to improve the coverage there.

> 2. I don't know which method to call in order to test the remove stack
> method.

You should call the |RemoveStack| method: https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/toolkit/components/telemetry/CombinedStacks.h#35

> 3. Should I use an `AutoJSContextWithGlobal` object for different calls
> (I've seen this in most of the tests from gtest/)? If yes, how do I find
> which one?
> Thanks!

For this test, there's no need to instantiate that object, so you don't need to worry about that. That object is used to provide a JS context when calling other JS-based Telemetry internal functions from the tests.

Comment 6

[Petru Gurita]

Attachment: TestCombinedStacks.cpp

This is my progress so far. When I try to print the number of stacks, mozilla crashes. I don't know how to solve the issue.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Petru Gurita from comment #6)
> Created attachment 8992483 [details]
> TestCombinedStacks.cpp
> 
> This is my progress so far. When I try to print the number of stacks,
> mozilla crashes. I don't know how to solve the issue.

Hey! Would you mind updating the ReviewBoard request or attaching a patch? It would make it easier to review and leave comments.

I can't see where you are trying to print the number of stacks in that code. Moreover, can you please give more information about where it's crashing? Do you have a stack?

Comment 8

[Petru Gurita]

I'm not at my personal computer right now and so I'm unable to attach a patch (changing the head to this particular commit will not work. If I run `hg log -u "Petru Gurita" ` the patch will not be shown). I tested the code again it crashes without even printing the number of stacks (the printing was done after adding the stacks). It crashes at `stacks.AddStack(s);`. Yes I do

Comment 9

[Petru Gurita]

Ok, I found out that was the problem:

ProcessedStack::Module m = {NS_LITERAL_STRING("test"), moduleName};
If I add this module to the stack the program crushes.

I don't know how to set a nsString value with a particular string value.
I also tried yesterday for few hours to figure it out but couldn't solve the issue.

Comment 10

[Petru Gurita]

When I try to add the changes to the path all I get is the message 'nothing changed'. I don't know what to do.

Comment 11

[Alessio Placitelli [:Dexter]]

(In reply to Petru Gurita from comment #10)
> When I try to add the changes to the path all I get is the message 'nothing
> changed'. I don't know what to do.

Which commands are you trying to run? Did you add the new file to the current changes using "hg add"?

Comment 12

[Petru Gurita]

Yes. I tried with and without `hg add`.

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Petru Gurita from comment #12)
> Yes. I tried with and without `hg add`.

What's the status of your tree? Check with |hg diff -c .|. You should be seeing the files you changed there. If not, you should probably hop on IRC and get help in #introduction :(

Comment 14

[Petru Gurita]

I'll eventually either find a solution or just submit a new patch, but the problem regarding assigning a string to a nsString still remains!

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Petru Gurita from comment #14)
> I'll eventually either find a solution or just submit a new patch, but the
> problem regarding assigning a string to a nsString still remains!

I'm getting away for some time, Jan-Erik will take over the mentoring for this bug! He'll help you take this to the last line :)

@Petru, are you still having difficulties with mercurial?

Comment 16

[Petru Gurita]

Yes I do.

Comment 17

[Petru Gurita]

Attachment: TestCombinedStacks.cpp

Comment 18

[Petru Gurita]

Comment on attachment 8994144 [details]
TestCombinedStacks.cpp

>/* Any copyright is dedicated to the Public Domain.
> * http://creativecommons.org/publicdomain/zero/1.0/
> */
>
>#include "CombinedStacks.h"
>#include "ProcessedStack.h"
>
>using namespace mozilla::Telemetry;
>using namespace TelemetryTestHelpers;
>
>TEST_F(TelemetryTestFixture, CombinedStacks) {
>
>  const size_t kMaxStacksKept = 10;
>  CombinedStacks stacks(kMaxStacksKept);
>
>  size_t iterations = kMaxStacksKept * 2;
>
>  //TEST ADD
>
>  for (size_t i = 0; i < iterations * 2; ++i) {
>
>    Telemetry::ProcessedStack s;
>    ProcessedStack::Frame f;
>    s.AddFrame(f);
>    stacks.AddStack(s);
>  }
>}

Comment 19

[Petru Gurita]

Attachment: Bug 1471144 - Add gtest coverage for CombinedStacks.

Added test coverage for methods `AddStack` and `RemoveStack`.

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

Comment 20

[Petru Gurita]

Attachment: Bug 1471144 - Add gtest coverage for 'CombinedStacks' .

Tested the functionality of 'CombinedStacks'. Added coverage for 'AddStack' and 'RemoveStack' methods.

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

Comment 21

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8995267 [details]
Bug 1471144 - Add gtest coverage for 'CombinedStacks' .

https://reviewboard.mozilla.org/r/259746/#review266920

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7c002c926ef
Add gtest coverage for 'CombinedStacks' . r=janerik

Comment 23

[Daniel Varga [:dvarga]]

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