1368713 - Test histogram_tools.py against historic Histograms.json formats

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Currently all testing for Histograms.json and the parsing (histogram_tools.py) is only focused on the client build.
However, we also use the file on the server-side through python_moztelemetry.

Currently its easy to forget about not breaking the server-side, resulting in e.g. bug 1364556.
In the server use, a flag is set in histogram_tools.py: strict_type_checks
https://dxr.mozilla.org/mozilla-central/search?q=strict_type_checks&redirect=false

We should add some basic coverage for that code path, against historic and current Histograms.json formats.

We should be careful to not impact build times too much though, maybe adding one test file with some variations of historic histogram definition formats is enough.

Comment 1

[Raajit Raj(:raajitr)]

I am interested.
But can you give more information?
What do you mean by 'historic and current Histograms'?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Alessio, can you sketch this out?

Comment 3

[Alessio Placitelli [:Dexter]]

I think we have a few options here, but first a few words on what 'historic and current histograms' means.

== Background ==
We define [1] the histograms we collect in the Histograms.json file [2]. The format of this file evolved with time, for example we recently added new mandatory properties to the histogram definitions. On the client-side, changing the Histogram.json file in a non-conformat way breaks the Firefox compilation, as we call histogram_tools.py [3] to generate some intermediate build files. Calling the histogram_tools file during compilation implies using the "strict_mode": if the file contains something we don't expect, we fail hard.

The very same file is used on the server side to help with the incoming data. There we need to be more relaxed, as histograms may still come from older versions of Firefox that adhere to an older format. This is why, on the server side, we enable a "relaxed" mode by setting |strict_type_checks| to False. This code path is currently not tested on the client side so, if we introduce some new feature that correctly works in strict mode, we might end up breaking non strict mode on the server, resulting in painful down time :-(

== What to test ==
With this bug we want to add some sort of test coverage for non strict mode. We might want to test at least that:

- adding new fields doesn't break anything;
- non-numeric expressions (e.g. "JS::gcreason::NUM_TELEMETRY_REASONS") for fields like "low", "high", "n_values", "n_buckets" don't break;
- that the most recent Histogram.json data work well in non-strict mode;

That should give us greater confidence that running this on the server won't blow thing sup.

== How to test ==
As I mentioned, we have a few options here.

1) Add a python test suite to Telemetry, which can be run with "./mach test". This implies adding a PYTHON_UNITTEST_MANIFESTS entry to our moz.build and defining a series of unit tests to cover our needs. This solution doesn't impact build times at all, as tests are ran separately (as we do with xpcshell, mochitests, gtests, ...). It also allows us to get fancy with the tests without checking in new Histogram.json files. This would also allow us to easily extend the test coverage to scalars and events in the future.

2) Add a new build step in moz.build (as GENERATED_FILES) that generates bogus output on each build and runs the 'gen-histogram-data.py' and 'gen-histogram-enum.py' on a set of old Histogram.json files that we'd have to check in. While I think this solution is easy to implement, it would clutter the moz.build file and impact build times a bit.

3) Add a "self-check" set of tests at the end of histogram_tools.py. This is basically just adding unit tests at the end of the histogram_tools.py file.

I would go for (1), as (2) and (3) don't feel like clean and flexible solutions in the long run.

@Georg, what's your take on this?

[1] - https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#declaring-a-histogram
[2] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json
[3] - https://dxr.mozilla.org/mozilla-central/search?q=histogram_tools.py&redirect=false

Comment 4

[Georg Fritzsche [:gfritzsche]]

(1) sounds solid to me, lets go with that.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Raajit Raj(:raajitr) from comment #1)
> I am interested.
> But can you give more information?
> What do you mean by 'historic and current Histograms'?

Raajit, thanks for being interested in this bug! I write about historic and current histogram, together with some background, in comment 3.

To implement the proposal (1) from comment 3, we can break the problem in two stages.

Part 1 - Enable the python unit tests for Telemetry

- Create a 'python' directory under toolkit/components/telemetry/tests/
- Add a 'python.ini' file there. The file will contain the name of the test files that will be run by the 'mach test' command. These are simply filenames wrapped in square brackets, e.g. [test_histogramtools_non_strict.py]
- Create the test file in the same directory, e.g. test_histogramtools_non_strict.py. The sample at [1] contains some of the boilerplate code needed to sketch out the test file.
- Add the new test manifest to the Telemetry moz.build file near [2]. It should be something like:

PYTHON_UNITTEST_MANIFESTS += [
    'tests/python/python.ini',
]

That should deal with the testing boilerplate. You should now be able to run './mach python-test toolkit/components/telemetry' and run the test code.

Part 2 - Testing histogram_tools.py

- We'd need to import histogram_tools in the test. The problem here is that the file lives in 'toolkit/components/telemetry' and not in the test directory. Let me know if you get stuck on this!
- We should add a test case for each scenario described in comment 3.
- Each scenario could, for example, provide a JSON blob containing the definition for a single histogram, parse the blob as done in [3] and check that the expected values are available.
- The 'current histograms' scenario could be as simple as calling 'histogram_tools.from_files', feeding the path to the Histogram.json file, and checking the expected values for some testing histogram e.g. TELEMETRY_TEST_COUNT

Raajit, please let me know if anything is unclear and do not hesitate to ask more questions if needed!

[1] - http://searchfox.org/mozilla-central/source/xpcom/idl-parser/xpidl/runtests.py#114
[2] - https://dxr.mozilla.org/mozilla-central/rev/cad53f061da634a16ea75887558301b77f65745d/toolkit/components/telemetry/moz.build#92
[3] - http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/toolkit/components/telemetry/histogram_tools.py#544-546

Comment 6

[Raajit Raj(:raajitr)]

Thanks Alessio for the brief description. 
I'm trying to implement proposal (1) and as expected I'm stuck at importing histogram_tools in the test file. Tried relative import but it isn't working, will have to dig little more.
Also, I'm unclear as to how to provide a JSON blob, should the JSON be in a seperate file or should I use something like this:
For e.g.:
    json.dumps({"jsonSyntax": true})

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Raajit Raj(:raajitr) from comment #6)
> Thanks Alessio for the brief description. 
> I'm trying to implement proposal (1) and as expected I'm stuck at importing
> histogram_tools in the test file. Tried relative import but it isn't
> working, will have to dig little more.

Cool, let me know if you get stuck on that. You might try adding the path to sys.path before importing histogram_tools and see if that works.

> Also, I'm unclear as to how to provide a JSON blob, should the JSON be in a
> seperate file or should I use something like this:
> For e.g.:
>     json.dumps({"jsonSyntax": true})

No need for a separate file, you can do as you described within the test that requires it :)

Comment 8

[Raajit Raj(:raajitr)]

Attachment: Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

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

Comment 9

[Raajit Raj(:raajitr)]

Hi,

I have uploaded a patch using the proposal (1) given by Alessio. It was an interesting bug to implement and will thank you both for helping me.

There are few issues where I think I need more input.

1. For `test_current_histogram`, I am trying to check the value in `record_in_processes` field using `test_histogram.record_in_processes()` which is giving me an error `Histogram instance has no attribute 'definition'`.

2. Am I implementing first two test cases correctly? Because my tests are passing even when I set `strict_type_check` to True in `histogram_tools.load_histograms_into_dict`.

Thanks again,

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Raajit Raj(:raajitr) from comment #9)
> Hi,
> 
> I have uploaded a patch using the proposal (1) given by Alessio. It was an
> interesting bug to implement and will thank you both for helping me.

Thank you for putting efforts in fixing this. It's a very important bug for us!

> There are few issues where I think I need more input.
> 
> 1. For `test_current_histogram`, I am trying to check the value in
> `record_in_processes` field using `test_histogram.record_in_processes()`
> which is giving me an error `Histogram instance has no attribute
> 'definition'`.

Congrats! You demonstrated why this bug is super important :) You stumbled upon a regression that was introduced in bug 1335343 (part 1) and that will be fixed in bug 1335343 (part 2). This is potentially landing today/tomorrow, so you should be set once that's fixed.

Without the test coverage you're adding we would have found about this problem too late: after breaking upstream :-)

> 2. Am I implementing first two test cases correctly? Because my tests are
> passing even when I set `strict_type_check` to True in
> `histogram_tools.load_histograms_into_dict`.

I'm leaving comments about this in the review.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review152784

This looks really good as a start, thank you Rajiit! Would you kindly also add a new test case for checking that duplicate keys don't break parsing in non-strict mode? e.g. repeat a field twice in the histogram definition

Please make sure your code passes Python linting. You can run the following command: ./mach lint -l flake8 toolkit/components/telemetry

::: commit-message-2a63a:1
(Diff revision 1)
> +Bug 1368713 - Test histogram_tools.py against historic Histograms.json formats for non strict mode

Could you change this first line to something like: 

Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

Then you can leave a blank line and add a few lines below to describe what your patch does (adds python test coverage, non-strict, etc.)

::: toolkit/components/telemetry/moz.build
(Diff revision 1)
>      'Telemetry.h',
>      'ThreadHangStats.h',
>  ]
>  
>  SOURCES += [
> -    'CombinedStacks.cpp',

The removal of line 56,57 and 60 in moz.build should not be in this patch. Maybe you updated the tree and made a mistake while rebasing?

::: toolkit/components/telemetry/moz.build:90
(Diff revision 1)
>      'ThirdPartyCookieProbe.jsm',
>      'UITelemetry.jsm',
>  ]
>  
>  TESTING_JS_MODULES += [
>    'tests/unit/TelemetryArchiveTesting.jsm',

nit: I know you didn't do this but, since we're here, would you kindly indent this line with 4 spaces instead of 2?

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:1
(Diff revision 1)
> +import json

Please add the Mozilla Public License header at the top of the file as done [here](http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/dom/bindings/mozwebidlcodegen/test/test_mozwebidlcodegen.py#1-3)

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:4
(Diff revision 1)
> +import json
> +import sys
> +from os import path
> +telemetry = path.abspath(path.dirname(path.dirname(path.dirname(__file__))))

```
sys.path.append(TELEMETRY_ROOT_PATH)
```

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:4
(Diff revision 1)
> +import json
> +import sys
> +from os import path
> +telemetry = path.abspath(path.dirname(path.dirname(path.dirname(__file__))))

Let's change this and the line below to explicitly tell what *dirname* is doing under the hood:

```
# Some comment describing why this is needed.
TELEMETRY_ROOT_PATH = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
```

This path will be used as a constant, so let's keep its name uppercase and make it more descriptive

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:6
(Diff revision 1)
> +import unittest
> +
> +import mozunit

Move all of the imports that don't need the path at the top of the file, sorted by the package name.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:12
(Diff revision 1)
> +
> +import mozunit
> +import histogram_tools
> +
> +class TestParser(unittest.TestCase):
> +    def test_adding_new_fields(self):

nit: maybe we could call this test test_unknown_field ?

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:13
(Diff revision 1)
> +import mozunit
> +import histogram_tools
> +
> +class TestParser(unittest.TestCase):
> +    def test_adding_new_fields(self):
> +        new_fields_histogram = {

nit: let's call it SAMPLE_HISTOGRAM (uppercase and more descriptive)

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:21
(Diff revision 1)
> +        def hook(ps):
> +            return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +        histograms = json.loads(json.dumps(new_fields_histogram), object_pairs_hook=hook)

Since you'll be using this lines in the test below, what about factoring them out in a separate function outside of TestParser?

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:23
(Diff revision 1)
> +                "description": "has accessibility support been instantiated",
> +                "new_field": "Its a new field"
> +                }}
> +        def hook(ps):
> +            return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +        histograms = json.loads(json.dumps(new_fields_histogram), object_pairs_hook=hook)

This will only give you an ordered dictionary, it will perform no real parsing other than loading the JSON into the dictionary.

You would still need to do something like:

```
hist = Histogram('A11Y_INSTANTIATED_FLAG', histograms['A11Y_INSTANTIATED_FLAG'], strict_type_checks=False)
```

And check the values returned by hist.kind(), etc. as you do in the last test.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:26
(Diff revision 1)
> +        def hook(ps):
> +            return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +        histograms = json.loads(json.dumps(new_fields_histogram), object_pairs_hook=hook)
> +
> +        self.assertIsNotNone(histograms['A11Y_INSTANTIATED_FLAG'].get('new_field'))
> +        self.assertEqual(histograms, new_fields_histogram)

Since we don't know how an unknown field will be handled, let's just test that it doesn't break parsing: please check that all the expected properties have the right values in the Histogram object.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:28
(Diff revision 1)
> +        histograms = json.loads(json.dumps(new_fields_histogram), object_pairs_hook=hook)
> +
> +        self.assertIsNotNone(histograms['A11Y_INSTANTIATED_FLAG'].get('new_field'))
> +        self.assertEqual(histograms, new_fields_histogram)
> +
> +    def test_non_numeric_expression_for_n_fields(self):

nit: test_non_numeric_expressions should be enough

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:29
(Diff revision 1)
> +
> +        self.assertIsNotNone(histograms['A11Y_INSTANTIATED_FLAG'].get('new_field'))
> +        self.assertEqual(histograms, new_fields_histogram)
> +
> +    def test_non_numeric_expression_for_n_fields(self):
> +        non_numeric_fields_histogram = {

nit: SAMPLE_HISTOGRAM?

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:32
(Diff revision 1)
> +
> +    def test_non_numeric_expression_for_n_fields(self):
> +        non_numeric_fields_histogram = {
> +            "A11Y_INSTANTIATED_FLAG": {
> +                "n_buckets": "JS::gcreason::NUM_TELEMETRY_REASONS",
> +                "low": "JS::gcreason::NUM_TELEMETRY_REASONS",

Let's also use `"mozilla::StartupTimeline::MAX_EVENT_ID"` for two fields.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:36
(Diff revision 1)
> +        def hook(ps):
> +            return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +        histograms = json.loads(json.dumps(non_numeric_fields_histogram), object_pairs_hook=hook)

You can use the re-factored function you defined for the previous test here.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:40
(Diff revision 1)
> +                }}
> +        def hook(ps):
> +            return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +        histograms = json.loads(json.dumps(non_numeric_fields_histogram), object_pairs_hook=hook)
> +
> +        value = "JS::gcreason::NUM_TELEMETRY_REASONS"

The parser will convert the expressions to numbers. You will need to check that the proper number is reported for the related field by the Histogram object. Check [here](http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/components/telemetry/histogram_tools.py#407-408) for the expected values.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:48
(Diff revision 1)
> +        self.assertEqual(histograms['A11Y_INSTANTIATED_FLAG']['low'], value)
> +        self.assertEqual(histograms['A11Y_INSTANTIATED_FLAG']['high'], value)
> +        self.assertEqual(histograms['A11Y_INSTANTIATED_FLAG']['n_values'], value)
> +
> +    def test_current_histogram(self):
> +        all_histograms = list(histogram_tools.from_files([path.join(telemetry,

Let's define

```
HISTOGRAMS_PATH = path.join(telemetry, "Histograms.json")

... histogram_tools.from_files([HISTOGRAMS_PATH], ...
```

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:55
(Diff revision 1)
> +                                                         strict_type_checks=False))
> +        test_histogram = [i for i in all_histograms if i.name() == 'TELEMETRY_TEST_FLAG'][0]
> +
> +        self.assertEqual(test_histogram.expiration(), 'never')
> +        self.assertEqual(test_histogram.kind(), 'flag')
> +        self.assertEqual(test_histogram.description(),

Let's not check the description but rather the record_in_processes, keyed (which should be False if not specified) and dataset in addition to the other 2 properties. See [here](https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html) for their meaning and values.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Comment on attachment 8876795 [details]
> Bug 1368713 - Test histogram_tools.py against historic Histograms.json
> formats for non strict mode
> 
> https://reviewboard.mozilla.org/r/148124/#review152784
> 
> This looks really good as a start, thank you Rajiit!

Also, sorry, I just noticed I misspelled your name :-\

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> (In reply to Raajit Raj(:raajitr) from comment #9)
> > Hi,
> > 
> > I have uploaded a patch using the proposal (1) given by Alessio. It was an
> > interesting bug to implement and will thank you both for helping me.
> 
> Thank you for putting efforts in fixing this. It's a very important bug for
> us!
> 
> > There are few issues where I think I need more input.
> > 
> > 1. For `test_current_histogram`, I am trying to check the value in
> > `record_in_processes` field using `test_histogram.record_in_processes()`
> > which is giving me an error `Histogram instance has no attribute
> > 'definition'`.
> 
> Congrats! You demonstrated why this bug is super important :) You stumbled
> upon a regression that was introduced in bug 1335343 (part 1) and that will
> be fixed in bug 1335343 (part 2). This is potentially landing
> today/tomorrow, so you should be set once that's fixed.

Raajit, the fix landed so you should be good to test record_in_processes now.

Comment 14

[Raajit Raj(:raajitr)]

> Congrats! You demonstrated why this bug is super important :) You stumbled
> upon a regression that was introduced in bug 1335343 (part 1) and that will
> be fixed in bug 1335343 (part 2). This is potentially landing
> today/tomorrow, so you should be set once that's fixed.
> 
> Without the test coverage you're adding we would have found about this
> problem too late: after breaking upstream :-)
 
I'm glad that my work paid off. Thanks again for your help. :)

Currently I am working on the fixes, and stuck at this point.
> The parser will convert the expressions to numbers. You will need to check
> that the proper number is reported for the related field by the Histogram
> object. Check
> [here](http://searchfox.org/mozilla-central/rev/
> d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/components/telemetry/
> histogram_tools.py#407-408) for the expected values.

Parsed numbers that I'm getting for "JS::gcreason::NUM_TELEMETRY_REASONS" is 3 and for "mozilla::StartupTimeline::MAX_EVENT_ID" is 2. I'm not sure if I'm doing something wrong here. This is my JSON Blob:

> SAMPLE_HISTOGRAM = {
>            "A11Y_INSTANTIATED_FLAG": {
>                "kind": "flag",
>                "description": "has accessibility support been instantiated",
>                "n_buckets": "JS::gcreason::NUM_TELEMETRY_REASONS",
>                "low": "JS::gcreason::NUM_TELEMETRY_REASONS",
>                "high": "mozilla::StartupTimeline::MAX_EVENT_ID",
>                "n_values": "mozilla::StartupTimeline::MAX_EVENT_ID"
>                }}

Also, its giving me an 'KeyError' on `Histogram('A11Y_INSTANTIATED_FLAG', histograms['A11Y_INSTANTIATED_FLAG'], strict_type_checks=False)` when I don't add both "kind" and "description" fields and I assume it should not break when using with type_checks = False. 
Here is the error trace if it helps: https://pastebin.mozilla.org/9025284

Comment 15

[Alessio Placitelli [:Dexter]]

Could you post your updated patch?

Comment 16

[Balaji]

Hi. If this bug is not yet fixed . I would like to work on this .

Comment 17

[Raajit Raj(:raajitr)]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148124/diff/1-2/

Comment 18

[Raajit Raj(:raajitr)]

Have uploaded the patch please check. thanks

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Balaji from comment #16)
> Hi. If this bug is not yet fixed . I would like to work on this .

Balaji, I'm sorry but Raajit is actively working on this bug already!

Comment 20

[Alessio Placitelli [:Dexter]]

(In reply to Raajit Raj(:raajitr) from comment #14)
> Parsed numbers that I'm getting for "JS::gcreason::NUM_TELEMETRY_REASONS" is
> 3 and for "mozilla::StartupTimeline::MAX_EVENT_ID" is 2. I'm not sure if I'm
> doing something wrong here. This is my JSON Blob:

That's because the histogram is of type "flag", which implicitly has 3 buckets.

> > SAMPLE_HISTOGRAM = {
> >            "A11Y_INSTANTIATED_FLAG": {
> >                "kind": "flag",
> >                "description": "has accessibility support been instantiated",
> >                "n_buckets": "JS::gcreason::NUM_TELEMETRY_REASONS",
> >                "low": "JS::gcreason::NUM_TELEMETRY_REASONS",
> >                "high": "mozilla::StartupTimeline::MAX_EVENT_ID",
> >                "n_values": "mozilla::StartupTimeline::MAX_EVENT_ID"
> >                }}
> 
> Also, its giving me an 'KeyError' on `Histogram('A11Y_INSTANTIATED_FLAG',
> histograms['A11Y_INSTANTIATED_FLAG'], strict_type_checks=False)` when I
> don't add both "kind" and "description" fields and I assume it should not
> break when using with type_checks = False. 
> Here is the error trace if it helps: https://pastebin.mozilla.org/9025284

Both "kind" and "description" are mandatory and should always be there regardless of the "strict_mode".
It's ok for you to add them into the SAMPLE definition!

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review158008

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:7
(Diff revision 2)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.import json
> +
> +import json
> +import mozunit
> +from os import path

Could you move this right after import unittest?

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:16
(Diff revision 2)
> +TELEMETRY_ROOT_PATH = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
> +sys.path.append(TELEMETRY_ROOT_PATH)
> +import histogram_tools
> +
> +def load_histogram(histograms):
> +    def hook(ps):

Would you kidnly add documentation for this function? You can follow the style from [here](http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/components/telemetry/parse_scalars.py#44)

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:41
(Diff revision 2)
> +        self.assertEqual(hist.kind(), 'flag')
> +        self.assertEqual(hist.record_in_processes(), ["main", "content"])
> +
> +    def test_non_numeric_expressions(self):
> +        SAMPLE_HISTOGRAM = {
> +            "A11Y_INSTANTIATED_FLAG": {

Let's change this to 

```
SAMPLE_HISTOGRAM = {
    "TEST_NON_NUMERIC_HISTOGRAM": {
        "kind": "linear",
        "description": "sample",
        "n_buckets": "JS::gcreason::NUM_TELEMETRY_REASONS",
        "high": "mozilla::StartupTimeline::MAX_EVENT_ID"
        }}
```

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:51
(Diff revision 2)
> +                "high": "mozilla::StartupTimeline::MAX_EVENT_ID",
> +                "n_values": "mozilla::StartupTimeline::MAX_EVENT_ID"
> +                }}
> +
> +        histograms = load_histogram(SAMPLE_HISTOGRAM)
> +        hist = histogram_tools.Histogram('A11Y_INSTANTIATED_FLAG',

Also rename this to TEST_NON_NUMERIC_HISTOGRAM since we're not using A11Y anymore.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:55
(Diff revision 2)
> +        histograms = load_histogram(SAMPLE_HISTOGRAM)
> +        hist = histogram_tools.Histogram('A11Y_INSTANTIATED_FLAG',
> +                                         histograms['A11Y_INSTANTIATED_FLAG'],
> +                                         strict_type_checks=False)
> +
> +        self.assertEqual(hist.n_buckets(), 101)

nit: please add a comment mentioning that the expected values come off histogram_tools.py

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:56
(Diff revision 2)
> +        hist = histogram_tools.Histogram('A11Y_INSTANTIATED_FLAG',
> +                                         histograms['A11Y_INSTANTIATED_FLAG'],
> +                                         strict_type_checks=False)
> +
> +        self.assertEqual(hist.n_buckets(), 101)
> +        self.assertEqual(hist.low(), 101)

Let's drop this check.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:58
(Diff revision 2)
> +                                         strict_type_checks=False)
> +
> +        self.assertEqual(hist.n_buckets(), 101)
> +        self.assertEqual(hist.low(), 101)
> +        self.assertEqual(hist.high(), 12)
> +        self.assertEqual(hist.n_values(), 12)

Let's drop this check.

Comment 22

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review158032

Does this pass the ./mach lint -f flake8 toolkit/components/telemetry linting?

Comment 23

[Raajit Raj(:raajitr)]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148124/diff/2-3/

Comment 24

[Raajit Raj(:raajitr)]

Hi Dexter, I have uploaded a patch and I think it will get r- for the docstring you requested. Can you please suggest me a better comment? I'm not good with these stuff. 

One more thing, I tried running the lint command for which I'm getting an error. Is there something I need to install?
https://pastebin.mozilla.org/9026185

Comment 25

[Alessio Placitelli [:Dexter]]

Hey Raajit, the pastebin looks empty. What's the error you're seeing? Make sure you pick a longer retention time on the pastebin (a month would do) so that we don't risk loosing the messages again :)

Comment 26

[Alessio Placitelli [:Dexter]]

Anyway, here's the output of the flake8 command on your latest patch:

> $ ./mach lint -l flake8 toolkit/components/telemetry/
> c:\mozilla-central\toolkit\components\telemetry\tests\python\test_histogramtools_non_strict.py
>   13:1  error  module level import not at top of file                              E402 (flake8)
>   15:1  error  expected 2 blank lines, found 1                                     E302 (flake8)
>   23:1  error  expected 2 blank lines, found 1                                     E302 (flake8)
>   70:1  error  expected 2 blank lines after class or function definition, found 1  E305 (flake8)
> 
> ? 4 problems (4 errors, 0 warnings)

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review159110

This looks like it's almost done, good job. Please make sure to also address the comments from the previous reviews, a few of them are still unaddressed.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:13
(Diff revision 3)
> +import unittest
> +from os import path
> +
> +TELEMETRY_ROOT_PATH = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
> +sys.path.append(TELEMETRY_ROOT_PATH)
> +import histogram_tools

nit: flake8 mentions that 2 blanks are expected after this, only one is found. Please add an additional blank line.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:13
(Diff revision 3)
> +import unittest
> +from os import path
> +
> +TELEMETRY_ROOT_PATH = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
> +sys.path.append(TELEMETRY_ROOT_PATH)
> +import histogram_tools

nit: please change this line to:

```
import histogram_tools  # noqa: E402
```

This will stop flake8 from complaining about including histogram_tools not at the top.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:21
(Diff revision 3)
> +    """Parse the passed Histogram and return a dictionary mapping histogram
> +    names to histogram parameters
> +    """
> +    def hook(ps):
> +        return histogram_tools.load_histograms_into_dict(ps, strict_type_checks=False)
> +    return json.loads(json.dumps(histograms), object_pairs_hook=hook)

nit: flake8 mentions that 2 blanks are expected after this, only one is found. Please add an additional blank line.

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:68
(Diff revision 3)
> +        test_histogram = [i for i in all_histograms if i.name() == 'TELEMETRY_TEST_FLAG'][0]
> +
> +        self.assertEqual(test_histogram.expiration(), 'never')
> +        self.assertEqual(test_histogram.kind(), 'flag')
> +        self.assertEqual(test_histogram.record_in_processes(), ["main", "content"])
> +        self.assertEqual(test_histogram.keyed(), False)

nit: flake8 mentions that 2 blanks are expected after this, only one is found. Please add an additional blank line.

Comment 28

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review159114

::: toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py:16
(Diff revision 3)
> +TELEMETRY_ROOT_PATH = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
> +sys.path.append(TELEMETRY_ROOT_PATH)
> +import histogram_tools
> +
> +def load_histogram(histograms):
> +    """Parse the passed Histogram and return a dictionary mapping histogram

While the comment is clear, you should really be following the style from [here](http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/components/telemetry/parse_scalars.py#44)

Comment 29

[Alessio Placitelli [:Dexter]]

Hey Raajit, are you stuck on something and need help?

Comment 30

[Raajit Raj(:raajitr)]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148124/diff/3-4/

Comment 31

[Raajit Raj(:raajitr)]

Hi Dexter, I'm really sorry for the delay I was caught up with some work and my internet was also not working last week (not making excuses).

I have uploaded the patch with changes, I hope everything will be fine now.

Thank you for your help. :)

Comment 32

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review162790

This looks good now, thanks.

Comment 33

[Alessio Placitelli [:Dexter]]

Comment on attachment 8876795 [details]
Bug 1368713 - Add test coverage for parsing historical Histograms.json formats

https://reviewboard.mozilla.org/r/148124/#review152784

> nit: I know you didn't do this but, since we're here, would you kindly indent this line with 4 spaces instead of 2?

Did you miss this change?

Comment 34

[Alessio Placitelli [:Dexter]]

(In reply to Raajit Raj(:raajitr) from comment #31)
> Hi Dexter, I'm really sorry for the delay I was caught up with some work and
> my internet was also not working last week (not making excuses).
> 
> I have uploaded the patch with changes, I hope everything will be fine now.
> 
> Thank you for your help. :)

No problem and no pressure! I was just asking to make sure you were not stuck on something, you don't need to explain ;) Stuff happens!

Also, don't mind comment 33. I triggered a try push now, if no problem is reported, we'll be good to land :)

Comment 35

[Alessio Placitelli [:Dexter]]

The python tests are executed automatically as part of the build job "B". Everything seems to look ok, so let's land this \o/

> [task 2017-07-17T08:22:29.241321Z] 08:22:29     INFO - /home/worker/workspace/build/src/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py
> [task 2017-07-17T08:22:29.241554Z] 08:22:29     INFO - TEST-PASS | /home/worker/workspace/build/src/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py | TestParser.test_current_histogram
> [task 2017-07-17T08:22:29.241796Z] 08:22:29     INFO - TEST-PASS | /home/worker/workspace/build/src/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py | TestParser.test_non_numeric_expressions
> [task 2017-07-17T08:22:29.242111Z] 08:22:29     INFO - TEST-PASS | /home/worker/workspace/build/src/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py | TestParser.test_unknown_field

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4da0146f568a
Add test coverage for parsing historical Histograms.json formats r=Dexter

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4da0146f568a