Closed Bug 1383793 Opened 3 years ago Closed 2 years ago

Add a test to make sure the whitelist is correctly for 'histogram_tools.py'

Categories

(Toolkit :: Telemetry, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Dexter, Assigned: jhu.jordan, Mentored)

References

Details

(Whiteboard: [measurement:client][lang=python][good-next-bug])

Attachments

(1 file, 2 obsolete files)

To prevent things as bug 1382556 from happening again, we should add a new test case at [1] to make sure that the whitelist file gets correctly loaded, parsed and used.
Mentor: alessio.placitelli
Whiteboard: [measurement:client][lang=python] → [measurement:client][lang=python][good-next-bug]
Is this bug still available ? If yes, then I would like to work on it.
(In reply to Cosm from comment #2)
> Is this bug still available ? If yes, then I would like to work on it.

Hey Cosm, yes, the bug is still available. I've just assigned it to you.

One possible approach for this would be to:

1 - Add a new test file like the one from comment 1, call it "test_histogramtools_strict.py"
1 - Add a test case in it, e.g. "test_whitelist".
2 - Use the "mock" library to provide fake data for the "Histograms.json" [2] and the "histogram-whitelists.json" [3] files. I would add a sample histogram definition with with no "bug_numbers" property for the first file and then add an exception for it in the fake data for the second file.
3 - I would then call the histogram_tools functions to read both files and make sure that the data is there, similarly to what's done in [4].

[1] - http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py#65
[2] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json
[3] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/histogram-whitelists.json
[4] - http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py#66-74
Assignee: nobody → kausam2015
Priority: P3 → P4
Hey Cosm,

do you need any more information or help?
Flags: needinfo?(kausam2015)
Hey Dexter,
I would like to work on this. If Cosm is not currently working on it.

Thanks
Flags: needinfo?(alessio.placitelli)
There was no activity on the bug recently, so I'm assuming he is not working on it. I've assigned the bug to you Raajit. Let me know if you need more details in addition to the ones from comment 3.
Assignee: kausam2015 → raajit.raj
Flags: needinfo?(kausam2015)
Flags: needinfo?(alessio.placitelli)
Raajit, are you stuck on something and need help? Are you still working on this?
Flags: needinfo?(raajit.raj)
Hello,
I am JaewoongYu. I just was wondering if I could get permission to fix this bug.
Hi JaewoongYu,

i assigned the bug to you. Let Alessio know if you need any help getting started!
Assignee: raajit.raj → yuj25
1 - Add a new test file like the one from comment 1, call it "test_histogramtools_strict.py"
1 - Add a test case in it, e.g. "test_whitelist".
2 - Use the "mock" library to provide fake data for the "Histograms.json" [2] and the "histogram-whitelists.json" [3] files. I would add a sample histogram definition with no "bug_numbers" property for the first file and then add an exception for it in the fake data for the second file.
3 - I would then call the histogram_tools functions to read both files and make sure that the data is there, similarly to what's done in [4].

Is this how to approach the way which I can fix this bug?
I can't add a new test file to Firefox Build Environment from comment 1.
(In reply to Jaewoongyu from comment #11)
> I can't add a new test file to Firefox Build Environment from comment 1.

What is the problem you're facing/what's the error?
Flags: needinfo?(raajit.raj) → needinfo?(yuj25)
I can find Mozilla-central/toolkit/components/telemetry/tests until here but it doesn't exist after that. And I am not sure that how to add test_histogramtools_strict.py this file to Firefox Build Environment.
Flags: needinfo?(yuj25)
(In reply to Jaewoongyu from comment #13)
> I can find Mozilla-central/toolkit/components/telemetry/tests until here but
> it doesn't exist after that. And I am not sure that how to add
> test_histogramtools_strict.py this file to Firefox Build Environment.

Do you mean you have no file under toolkit/components/telemetry/tests/ locally? You should have files and sub directories as shown here: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests

If you don't, you might want to download the codebase again.
Flags: needinfo?(yuj25)
I can't see the file locally after this Mozilla-central/toolkit/components/telemetry/tests
What do you mean by download the codebase again exactly? and How can I download the codebase again? Could you please explain to me more specifically?
Flags: needinfo?(yuj25)
(In reply to Jaewoongyu from comment #15)
> I can't see the file locally after this
> Mozilla-central/toolkit/components/telemetry/tests

I see. Did you delete them accidentally?

> What do you mean by download the codebase again exactly? and How can I
> download the codebase again? Could you please explain to me more
> specifically?

I mean downloading the code again. If you're using mercurial, one thing you could do is "hg update --clean" and see if the files pop back again. Otherwise, you might want to clone the repository again.
Yeah, I have downloaded and have found it. Now it is working properly. Could you please guide me "Add a test case in it, e.g. "test_whitelist"" this part with more detail?
(In reply to Jaewoongyu from comment #17)
> Yeah, I have downloaded and have found it. Now it is working properly. Could
> you please guide me "Add a test case in it, e.g. "test_whitelist"" this part
> with more detail?

A test case is a member function of the test class, as done with the current tests here: https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py#29

See |test_unknown_fields|.
Could you give me more hint to do it?
(In reply to Jaewoongyu from comment #19)
> Could you give me more hint to do it?

Sure, are you stuck on something specific?
Unassigning due to inactivity.
Assignee: yuj25 → nobody
Hi! I'd like to fix this bug if it's available.
It is available! Please let me know if you have any questions.
Assignee: nobody → jhu.jordan
Mentor: chutten
Status: NEW → ASSIGNED
I have a question regarding histogram-whitelist.json[1]. Are these required fields for the specified histogram? For example `A11Y_CONSUMERS` (specified in [2]) requires a "expiry_default" field.

[1] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/histogram-whitelists.json#1517
[2] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#11
There are several whitelists, and they have different meanings:
alert_emails - for histograms missing the "alert_emails" field (which we require for all new histograms)
bug_numbers - for histograms missing the "bug_numbers" field (which we require for all new histograms)
n_buckets - for histograms using more than 100 buckets (because more buckets = more space, bandwidth, cost)
expiry_default - for histograms saying "expires_in_version": "default" (new histograms use "expires_in_version": "never")
kind - for histograms that are of deprecated types "flag" or "count" (those should be bool or uint scalars these days)

If you search for "whitelists[" in parse_histogram, you'll see how they're used.

(( If you're wondering, the idea behind the whitelists is that no new histograms should be added to them. They are just for allowing older histograms to continue to exist unchanged until they are removed. ))

Does that help?
Ah I think I get it. As you mentioned, the whitelists are there to support older histograms that do not have the newer fields.

For example, if a new histogram was created without a bug_numbers field it would not be allowed as noted at [1]. However, if an old one was instantiated without the field and was on the whitelist, it would be a valid histogram. Let me know if that's correct.

[1] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/parse_histograms.py#429
That is correct. It's not just missing fields, though, it's also checking values of some fields.

As such, it's important we test it. Comment #3 has a nice outline of how you might want to approach this.
How can I import histogram_tools?
histogram_tools is now called parse_histograms.py (it was renamed last November)
Is there a good way to assert that the ParserError had no eventual_errors while instantiating the histogram[1]? Currently I'm checking that ParserError.eventual_errors has a length of 0. I'm asking because even if there is an issue with the histogram, no error is thrown when I instantiate it and my test case passes.

[1] - https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/shared_telemetry_utils.py#39
There should be an eventual error. It's possible that, with no atexit handler registered in your test, ParserError will never actually throw any of the errors, but it should have stored one.
How is this going? Did you find the eventual errors?
Flags: needinfo?(jhu.jordan)
Hi! Sorry for the late response. How would I register an atexit handler within the testcase so that the ParserError is thrown? I'm using the same format as: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/python/test_histogramtools_non_strict.py
Flags: needinfo?(jhu.jordan)
I don't know that you'll need to register the handler, is eventual_errors not available as ParserError.eventual_errors?
Oh yes it is. So is checking the length ParserError.eventual_errors after instantiating the histogram the best way to check if any errors occured?
It might be. Or it might make more sense to check that the loaded and parsed histograms are correctly included or excluded based on the whitelists.

Consider maybe a flow like test_histograms_non_strict.py uses: Create a fake histogram definition, then try loading and parsing it. Once you have that working for a normal histogram, try using a histogram that fails one or more of the whitelists and make sure it doesn't load.

Then you can provide a fake whitelist that contains your failing histogram, and you can test that it passes.

Does this make sense?
MozReview-Commit-ID: FAD6fiMbPWx
Attachment #8964426 - Flags: review?(alessio.placitelli)
Comment on attachment 8964426 [details] [diff] [review]
Added test to make sure whitelist is used correctly for histogram_tools.py

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

Hi Jordan! Great job up to now, thanks for your patch! This perfectly covers the "bug_numbers" field. I'd love to see coverage for the other whitelistable fields as well, if possible. Just keep up the good work you did so far ;)

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py
@@ +50,5 @@
> +        }
> +        histograms = load_histogram(SAMPLE_HISTOGRAM)
> +        parse_histograms.load_whitelist()
> +
> +        hist = parse_histograms.Histogram('TEST_WHITELIST_HISTOGRAM',

nit: |hist| is assigned but never used, so no need to assign it otherwise linting will complain ;) To manually perform linting on this python file, run ./mach lint -l flake8 toolkit/components/telemetry/tests/python/

@@ +57,5 @@
> +
> +        self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +    def test_histogram_on_whitelist(self):
> +        SAMPLE_HISTOGRAM = {

Good job here, that's the right approach. In addition to checking the |bug_numbers| field, let's also check the other whitelisted fields: alert_emails, n_buckets, expiry_default, kind. Let's rename |SAMPLE_HISTOGRAM| to |SAMPLE_HISTOGRAMS| and add an example histogram for every whitelisted exception. If you think that works better, you can have separate test cases: one for every whitelisted field. That would reduce the code complexity for each test case. As you prefer :)

@@ +91,5 @@
> +        self.assertEqual(hist.keyed(), False)
> +
> +        parse_histograms.whitelists = None
> +
> +if __name__ == '__main__':

nit: we need 2 blank lines between this and the previous block of code, so let's add another empty line! (and check linting again after this change)
Attachment #8964426 - Flags: review?(alessio.placitelli) → feedback+
Thanks for the feedback! Would it make sense to group them by field names like so:

test_whitelist_histogram_bug_numbers_field:
- Check histogram with no bug_numbers that IS NOT on whitelist
- Check histogram with no bug_numbers that IS on whitelist

test_whitelist_histogram_alert_emails_field:
- Check histogram with no alert_emails that IS NOT on whitelist
- Check histogram with no alert_emails that IS on whitelist

etc.

I like the idea having separate test cases as you suggested but there might be too many cases if I separate them by valid and invalid histograms as I have currently. Let me know if this makes sense.
Flags: needinfo?(alessio.placitelli)
(In reply to Jordan Hu from comment #39)
> Thanks for the feedback! Would it make sense to group them by field names
> like so:
> 
> test_whitelist_histogram_bug_numbers_field:
> - Check histogram with no bug_numbers that IS NOT on whitelist
> - Check histogram with no bug_numbers that IS on whitelist
> 
> test_whitelist_histogram_alert_emails_field:
> - Check histogram with no alert_emails that IS NOT on whitelist
> - Check histogram with no alert_emails that IS on whitelist
> 
> etc.
> 
> I like the idea having separate test cases as you suggested but there might
> be too many cases if I separate them by valid and invalid histograms as I
> have currently. Let me know if this makes sense.

Yes, this makes a lot of sense! Please go ahead and let me know if you have any other question ;)
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8966818 [details]
Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py

https://reviewboard.mozilla.org/r/233164/#review241224


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:54
(Diff revision 1)
> +            }
> +        }
> +        histograms = load_histogram(SAMPLE_HISTOGRAM)
> +        parse_histograms.load_whitelist()
> +
> +        hist = parse_histograms.Histogram('TEST_WHITELIST_HISTOGRAM',

Error: Local variable 'hist' is assigned to but never used [flake8: F841]

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:95
(Diff revision 1)
> +        self.assertEqual(hist.record_in_processes(), ["main", "content"])
> +        self.assertEqual(hist.keyed(), False)
> +
> +        parse_histograms.whitelists = None
> +
> +if __name__ == '__main__':

Error: Expected 2 blank lines after class or function definition, found 1 [flake8: E305]
Comment on attachment 8966819 [details]
Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py

https://reviewboard.mozilla.org/r/235498/#review241240

Hey Jordan, would you kindly merge the two patches and then flag me for review on the merged commit? You can use the `hg histedit` command. While we're here, let's fix the problems from comment 43 ;)
Attachment #8966819 - Flags: review?(alessio.placitelli) → review-
Attachment #8966818 - Flags: review?(chutten)
Oops! I've fixed in it in the updated commit. I think I have 3 review requests pending. Should I remove the two older ones?
Attachment #8964426 - Attachment is obsolete: true
(In reply to Jordan Hu from comment #45)
> Oops! I've fixed in it in the updated commit. I think I have 3 review
> requests pending. Should I remove the two older ones?

I've removed the oldest one for you. The remaining two need to be merged: right now the updated one stacks upon the old one :)
Attachment #8966818 - Attachment is obsolete: true
Comment on attachment 8966819 [details]
Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py

https://reviewboard.mozilla.org/r/235498/#review241706

Hi Jordan! Thanks for the updated patch, looks like we're almost there. The requested changes are mostly nits, except for the test coverage for the "flag" kind, which is missing and should be added. Other than that, great job, I think that next round would probably be the final one ;)

::: commit-message-7ea9d:1
(Diff revision 2)
> +Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py r=Dexter

nit: you don't have to change this, it's more of a reference for future bugs. Note that "r=Dexter" implies that I reviewed and gave an r+ on this patch. The correct way to write that you're requesting a review in the commit message is "r?Dexter" :)

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:41
(Diff revision 2)
> +        ParserError.exit_func()
> +        self.assertTrue(hist.expiration(), "never")
> +        self.assertTrue(hist.kind(), "boolean")
> +        self.assertTrue(hist.record_in_processes, ["main", "content"])
> +
> +    def test_strict_whitelist_histogram_bug_numbers(self):

nit: I'd keep it short and call this `test_missing_bug_numbers`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:60
(Diff revision 2)
> +                                   histograms['TEST_HISTOGRAM_WHITELIST_BUG_NUMBERS'],
> +                                   strict_type_checks=True)
> +
> +        self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +        # set global whitelists for parse_histograms

nit: start the sentence with a capital "S" and end it with a "." :)

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:83
(Diff revision 2)
> +        self.assertEqual(hist.record_in_processes(), ["main", "content"])
> +        self.assertEqual(hist.keyed(), False)
> +
> +        parse_histograms.whitelists = None
> +
> +    def test_strict_whitelist_histogram_alert_emails(self):

nit: I'd keep it short and call this `test_missing_alert_emails`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:102
(Diff revision 2)
> +                                   histograms['TEST_HISTOGRAM_WHITELIST_ALERT_EMAILS'],
> +                                   strict_type_checks=True)
> +
> +        self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +        # set global whitelists for parse_histograms

nit: same as the other test case

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:125
(Diff revision 2)
> +        self.assertEqual(hist.record_in_processes(), ["main", "content"])
> +        self.assertEqual(hist.keyed(), False)
> +
> +        parse_histograms.whitelists = None
> +
> +    def test_strict_whitelist_histogram_n_buckets(self):

nit: I'd keep it short and call this `test_high_n_buckets`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:148
(Diff revision 2)
> +                                   histograms['TEST_HISTOGRAM_WHITELIST_N_BUCKETS'],
> +                                   strict_type_checks=True)
> +
> +        self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +        # set global whitelists for parse_histograms

nit: same :)

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:174
(Diff revision 2)
> +        self.assertEqual(hist.high(), 16777216)
> +        self.assertEqual(hist.n_buckets(), 200)
> +
> +        parse_histograms.whitelists = None
> +
> +    def test_strict_whitelist_histogram_expiry_default(self):

nit: `test_expiry_default`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:194
(Diff revision 2)
> +                                   histograms['TEST_HISTOGRAM_WHITELIST_EXPIRY_DEFAULT'],
> +                                   strict_type_checks=True)
> +
> +        self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +        # set global whitelists for parse_histograms

nit: as the other test cases

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:217
(Diff revision 2)
> +        self.assertEqual(hist.record_in_processes(), ["main", "content"])
> +        self.assertEqual(hist.keyed(), False)
> +
> +        parse_histograms.whitelists = None
> +
> +    def test_stric_whiteslist_histogram_kind(self):

nit: `test_unsupported_kind_count`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:222
(Diff revision 2)
> +    def test_stric_whiteslist_histogram_kind(self):
> +        SAMPLE_HISTOGRAM = {
> +            "TEST_HISTOGRAM_WHITELIST_KIND": {
> +                "record_in_processes": ["main", "content"],
> +                "expires_in_version": "never",
> +                "kind": "count",

You would also need to add test coverage for the "flag" kind. You can add a new test case below and call it `test_unsupported_kind_flag`

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:232
(Diff revision 2)
> +            }
> +        }
> +        histograms = load_histogram(SAMPLE_HISTOGRAM)
> +        parse_histograms.load_whitelist()
> +
> +        # parse_histograms.Histogram('TEST_HISTOGRAM_WHITELIST_KIND',

Why is this different than the other test cases? Would you mind making it as the others/get rid of the commented lines?

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:240
(Diff revision 2)
> +        self.assertRaises(SystemExit, parse_histograms.Histogram,
> +                          'TEST_HISTOGRAM_WHITELIST_KIND',
> +                          histograms['TEST_HISTOGRAM_WHITELIST_KIND'],
> +                          strict_type_checks=True)
> +
> +        # self.assertRaises(SystemExit, ParserError.exit_func)

As above.

::: toolkit/components/telemetry/tests/python/test_histogramtools_strict.py:242
(Diff revision 2)
> +                          histograms['TEST_HISTOGRAM_WHITELIST_KIND'],
> +                          strict_type_checks=True)
> +
> +        # self.assertRaises(SystemExit, ParserError.exit_func)
> +
> +        # set global whitelists for parse_histograms

nit: as the fixed in the other test cases.
Attachment #8966819 - Flags: review?(alessio.placitelli) → review-
Comment on attachment 8966819 [details]
Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py

https://reviewboard.mozilla.org/r/235498/#review241706

> Why is this different than the other test cases? Would you mind making it as the others/get rid of the commented lines?

The check for the "kind" field throws a ParserError.handle_now() rather than a ParserError.handle_error() like the other fields: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/parse_histograms.py#414. Because of this, I need to capture the Exception on the Histogram instantiation. I'll delete those commented out lines!
Comment on attachment 8966819 [details]
Bug 1383793 Added test to make sure whitelist is used correctly for histogram_tools.py

https://reviewboard.mozilla.org/r/235498/#review242522

This looks great to me now, thanks!
Attachment #8966819 - Flags: review?(alessio.placitelli) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/42aca1fc0833
Added test to make sure whitelist is used correctly for histogram_tools.py r=Dexter
https://hg.mozilla.org/mozilla-central/rev/42aca1fc0833
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.