Make Autotest-based tests that consume Chrome histograms less fragile |
|||||||||
Issue descriptionAs tracked by issue 844121 , a bunch of video decode Autotest-based tests started failing yesterday on the Chrome PFQ builders. These tests use client/cros/video/histogram_verifier.py in the autotest repo to parse chrome://histograms/<histogram-name> . It turns out that https://crrev.com/c/890627 caused Chrome to only report partial histogram results (issue 844341). This was fixed by https://crrev.com/c/1065462. Autotest-based tests like these seem very fragile to me. After discussion with Steven, I think we should do the following: a) Add a Chrome unit test to make sure that the format used by Chrome for chrome://histograms/<histogram-name> doesn't change. Per the above change, it sounds like there are plans to update Chrome's UI. If somebody does that, they need to be aware that there are downstream consumers of these pages. b) Add an Autotest-based test that can run on VMs (rather than just on real hardware, like the video tests) that verifies that histogram_verifier.py is able to read an always-expected-to-be-there histogram from Chrome. This will let us catch failures more quickly (and ideally on the Chrome CQ instead of PFQ, once Chrome OS VMs are running there). c) Update histogram_verifier.py to report better errors. Right now, it uses the error message "<histogram-name> not loaded or histogram bucket not found or histogram bucket found at < 100%". At the very least, it should say which of these three cases matches reality.
,
May 21 2018
I think we can get histograms in JSON form. I just saw a discussion thread years ago: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-dev/hD60eWK1_p4/Sk__EKBuAgAJ The latest broken issue mentioned above was because of a bug in histogram page rendering code. It is not because of rendering page change. In fact histogram_verifier.py is quite robust to rendering page change as it process raw text rather than html text. Though make it consume JSON object could be even better. a) I think the right way to do is to ask consumer of Chrome histogram to parse JSON object. But we still have to assume Histogram JS API implement correctly. b) This is worth to try. But for the specific bug, it affect some but not all histograms. How about Chromium Histograms developer adding a unittest to cover the issue? c) I can improve the error message of histogram_verifier.py
,
May 21 2018
,
May 21 2018
I believe we're using histograms to verify that we've triggered some internal chrome state, and this seems fragile and non-obvious for anyone making upstream changes. It'll be easy to inadvertently 'fix' the unit test, and I don't know that we should be wasting time on the chromium waterfall running tests solely to ensure that our hacky backchannel isn't broken. We'll expand the sanity tests we run on the chromium waterfall, and we'll have a small time budget to work with. We should use a private extension api, or navigate to some internal test page that dumps the data, or something along these lines. I think an explicit test page/method is preferable to piggy-backing on histograms.
,
Jul 4
These tests are still problematic (see issue 860135 ). Please make improving them a priority.
,
Jul 5
Issue 860135 has been merged into this issue.
,
Jul 5
Issue 804221 has been merged into this issue.
,
Jul 5
First step, let's improve error message https://crbug.com/860135 mentioned.
,
Jul 27
,
Jul 27
Refer https://crbug.com/868069#c5 , sometimes Telemetry's tab.Navigate() raised exception, histogram_verifier.verify() should handle this.
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/34be76d910668aaf6fb44ef8875f5dd27da422c2 commit 34be76d910668aaf6fb44ef8875f5dd27da422c2 Author: Dean Liao <deanliao@chromium.org> Date: Fri Jul 27 03:45:08 2018 Style fix to make pylint happy. BUG= chromium:844755 TEST=Run utils_unittest.py Change-Id: I5bfbf7e3a8b46f2fc166d317c55956384796e787 Reviewed-on: https://chromium-review.googlesource.com/1142047 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Shuo-Peng Liao <deanliao@google.com> Reviewed-by: Shuo-Peng Liao <deanliao@google.com> [modify] https://crrev.com/34be76d910668aaf6fb44ef8875f5dd27da422c2/client/common_lib/utils.py [modify] https://crrev.com/34be76d910668aaf6fb44ef8875f5dd27da422c2/client/common_lib/utils_unittest.py
,
Jul 31
Refer https://crbug.com/845372#c3 , we should verify histogram diff before and after test to avoid false negative/positive result because of unexpected histogram state before the test.
,
Jul 31
,
Aug 9
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/7d51db0a5b55aafffe9b3022db1dc825e86b2a9f commit 7d51db0a5b55aafffe9b3022db1dc825e86b2a9f Author: Dean Liao <deanliao@chromium.org> Date: Thu Aug 09 09:21:38 2018 client/utils: Add poll_for_condition_ex() and improve TimeoutError. Add poll_for_condition_ex() which can handle exception condition() raised and embed it into TimeoutError so that caller can know what causes condition() fails to poll. Improve TimeoutError so that it can embed a reason object, which can be either a string or an exception. Add Timer class to move out timer locic from poll_for_condition_ex(). BUG= chromium:844755 TEST=Run utils_unittest.py CQ-DEPEND=CL:1142047 Change-Id: Ic7ee3ee5f2c5da722bf132303ac0d694d77a527f Reviewed-on: https://chromium-review.googlesource.com/1138040 Commit-Ready: Shuo-Peng Liao <deanliao@google.com> Tested-by: Shuo-Peng Liao <deanliao@google.com> Reviewed-by: Allen Li <ayatane@chromium.org> [modify] https://crrev.com/7d51db0a5b55aafffe9b3022db1dc825e86b2a9f/client/common_lib/utils.py [modify] https://crrev.com/7d51db0a5b55aafffe9b3022db1dc825e86b2a9f/client/common_lib/utils_unittest.py
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/a2dbbfcc5e9d96dbe1dea5d64ff6ed4106a4a1ce commit a2dbbfcc5e9d96dbe1dea5d64ff6ed4106a4a1ce Author: Dean Liao <deanliao@chromium.org> Date: Thu Aug 23 17:53:17 2018 client/utils: Add poll_for_condition_ex() and improve TimeoutError. Add poll_for_condition_ex() which can handle exception condition() raised and embed it into TimeoutError so that caller can know what causes condition() fails to poll. Improve TimeoutError so that it can embed a reason object, which can be either a string or an exception. Add Timer class to move out timer locic from poll_for_condition_ex(). BUG= chromium:844755 TEST=Run utils_unittest.py CQ-DEPEND=CL:1142047 Change-Id: Ic7ee3ee5f2c5da722bf132303ac0d694d77a527f Previous-Reviewed-on: https://chromium-review.googlesource.com/1138040 (cherry picked from commit 3fbb8b3a4c7b5694abe1ea3c38a16cd867bb1e50) Reviewed-on: https://chromium-review.googlesource.com/1186923 Tested-by: Shuo-Peng Liao <deanliao@google.com> Reviewed-by: Allen Li <ayatane@chromium.org> Commit-Queue: Shuo-Peng Liao <deanliao@google.com> [modify] https://crrev.com/a2dbbfcc5e9d96dbe1dea5d64ff6ed4106a4a1ce/client/common_lib/utils.py [modify] https://crrev.com/a2dbbfcc5e9d96dbe1dea5d64ff6ed4106a4a1ce/client/common_lib/utils_unittest.py
,
Aug 27
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b commit efaabc2693ccbabac48dc4b3f38c2fefec8ae87b Author: Daniel Erat <derat@chromium.org> Date: Wed Sep 05 21:03:36 2018 chromeos: Add autotestPrivate.getHistogram function. Add a getHistogram function to the chrome.autotestPrivate that Chrome OS integration tests can call to get data about a given histogram. This will hopefully be more reliable than the current approach of parsing chrome://histograms. Bug: 878641 , 844755 Change-Id: I9077c297a69742b9e225eefed52fe4b012d796fa Reviewed-on: https://chromium-review.googlesource.com/1200840 Commit-Queue: Dan Erat <derat@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Cr-Commit-Position: refs/heads/master@{#588996} [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/chrome/browser/extensions/api/autotest_private/autotest_private_api.cc [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/chrome/browser/extensions/api/autotest_private/autotest_private_api.h [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/chrome/common/extensions/api/autotest_private.idl [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/chrome/test/data/extensions/api_test/autotest_private/test.js [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/extensions/browser/extension_function_histogram_value.h [modify] https://crrev.com/efaabc2693ccbabac48dc4b3f38c2fefec8ae87b/tools/metrics/histograms/enums.xml |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by posciak@chromium.org
, May 21 2018