|
|
DescriptionImplemented telemetry benchmark that loads page and outputs screenshot.
Logic for '--disable-javascript' flag not implemented yet.
BUG=536285
Review-Url: https://codereview.chromium.org/2923163007
Cr-Commit-Position: refs/heads/master@{#479558}
Committed: https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f830c6ed86c9db
Patch Set 1 #
Total comments: 20
Patch Set 2 : Changed name to Screenshot, added logging, error handling, more descriptive comments #
Total comments: 7
Patch Set 3 : Moved files to tools/perf/contrib/cluster_telemetry, more logging, detecting if run on Linux in uniā¦ #
Total comments: 10
Patch Set 4 : Removed mtime, saved_picture_count, javascript flag, Edited unittest #
Total comments: 3
Patch Set 5 : More detailed logging, unit test now returns if not Linux #
Total comments: 6
Patch Set 6 : Warning level logs, added decorators.enabled() to unit test #Patch Set 7 : Unit test debug #Patch Set 8 : Debugging #Patch Set 9 : Edited gn_isolate_map #Patch Set 10 : Edited chromium.fyi.json #Patch Set 11 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into pixel-diff-screenshot #Patch Set 12 : Edited chromium.linux.json #Patch Set 13 : Edited chromium.linux.json #Patch Set 14 : Changed screenshot_supported logic in inspector_backend #Patch Set 15 : Rebase #Patch Set 16 #Patch Set 17 : Addex pixel assert to unit test #
Messages
Total messages: 81 (55 generated)
Description was changed from ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. BUG=536285 ========== to ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. BUG=536285 ==========
lchoi@chromium.org changed reviewers: + nednguyen@google.com, rmistry@chromium.org, wkorman@chromium.org
Exciting! My feedback is all essentially either to enhance comments or improve error handling. We need feedback from nednguyen@ and rmistry@ on how telemetry and CT would like to handle. https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... File tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:14: """Captures PNG screenshots of web pages for Cluster Telemetry.""" Consider adding note that we write to a local file with filename turned into path-safe url, and that we rely on CT to aggregate. https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:28: parser.add_option('--disable-javascript', action='store_true', Either remove for now or clarify that this is planned but not yet implemented in the description. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... File tools/perf/measurements/pixel_diff.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:4: import os +1 space above https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:12: class PixelDiff(legacy_page_test.LegacyPageTest): Add doc comment describing purpose. See power.py which has a brief one as an example. Perhaps note file output/naming similar to my comment on the other script. I also considered whether we should instead name this script 'screenshot' since that's more self documenting. Could make a case either way. Let's see opinion of our other reviewers. Note there is already as we've discussed in email a telemetry/util/screenshot.py script so a benefit of naming as we have here is lack of ambiguity. Also, perhaps we'd do more/different things for pixel diff down the line, so having that specific naming is better. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:18: self._disable_javascript = disable_javascript Same as previous: remove for now or comment to note not yet implemented. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:20: def ValidateAndMeasurePage(self, page, tab, results): Do we have sufficient useful log output when we run this script with '-v' to make scrutiny if we have to debug later useful? Or should we add some logging.info calls to make it easier to see what the input params were, what path we try to write to, etc. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:28: pass I see this is what rasterize_and_record_micro does as well. Two thoughts: (1) I would think we'd at least want to log something here. Two places where tools/perf logs timeouts currently for example: page_sets/system_health/multi_tab_stories.py contrib/cros_benchmarks/tab_switching_stories.py (2) seeking feedback from other reviewers: if we time out waiting for document ready, do we really still want to go ahead and try to take a screenshot? Or should we just report an error? Is it sufficient to just output results dict with 'saved_picture_count' of 0? How does Cluster Telemetry logic that calls into this script want to handle failure/error like this? Concern here is to ensure we (a) always check error results, and (b) make sure we do what we can to make debugging in telemetry/cluster-telemetry logs easy for inevitable failures. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:32: os.makedirs(self._png_outdir) If we fail to makedir (and probably some other os calls here) it will raise an exception. Is it ok for that case to propagate directly and cause script to just fail w/ stack trace? See my question (2) above re: how CT wants to deal with error handling. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:39: screenshot = tab.Screenshot() This can raise a few exceptions, should we catch and handle in any manner? Again, need to hear from other reviewers on (2) above. https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:45: image_util.WritePngFile(screenshot, outpath) Error handling -- it looks like this can throw some exceptions. It also looks like one code path calls into cv2 python lib which actually has a retval bool which presumably reports success/failure and is ignored in the image_util lib but the docs are poor, see: http://docs.opencv.org/2.4/modules/highgui/doc/reading_and_writing_images_and... The other code path uses built in png lib which also has unclear docs on error situation: https://pythonhosted.org/pypng/png.html#png.Writer.write_array I'm not sure what code path we are actually using. Could add logging to image_util.py to see: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... In any case I would say it's ok to add a TODO here and potentially file bug to enhance error handling of image_util.WritePngFile with unit tests separately. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... File tools/perf/measurements/pixel_diff_unittest.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff_unittest.py:4: import shutil +1 blank above https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff_unittest.py:22: ps = self.CreateStorySetFromFileInUnittestDataDir('blank.html') Rename ps -> page_set for better understandability (even though I see it's 'ps' in the base test class too) https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff_unittest.py:26: # Screenshots are not supported on all platforms We should make sure this test fails somehow if for example we somehow get into a state, due to subsequent change by someone else, where screenshots are not supported on any platform. I could be convinced that it's ok given our primary focus of having this run on linux to semi hard code this test to check whatever is appropriate to ensure that, on linux, screenshots work. Also I wanted to understand how this actually screenshots for unit test purposes since I don't think it's actually bringing up a content shell or browser. Where is the fake that for at least one platform pretends it really took a screenshot? Update from in person discussion: nednguyen@ it looks like PageTestTestCase is actually more of a base class for integration tests because it does actually start up a browser. Just wanted to confirm this is reasonable and expected.
Some high level comments https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... File tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:13: class PixelDiffCT(perf_benchmark.PerfBenchmark): This should inherit s.t like a base CTBenchmark class so you have less boiler plate to fill out https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... File tools/perf/measurements/pixel_diff.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:28: pass On 2017/06/07 20:15:11, wkorman wrote: > I see this is what rasterize_and_record_micro does as well. Two thoughts: > > (1) I would think we'd at least want to log something here. > > Two places where tools/perf logs timeouts currently for example: > > page_sets/system_health/multi_tab_stories.py > contrib/cros_benchmarks/tab_switching_stories.py > > (2) seeking feedback from other reviewers: if we time out waiting for document > ready, do we really still want to go ahead and try to take a screenshot? Or > should we just report an error? Is it sufficient to just output results dict > with 'saved_picture_count' of 0? How does Cluster Telemetry logic that calls > into this script want to handle failure/error like this? Concern here is to > ensure we (a) always check error results, and (b) make sure we do what we can to > make debugging in telemetry/cluster-telemetry logs easy for inevitable failures. I think if we timeout here, it's better to report error or something like "I can't process this" gracefully.
rmistry@google.com changed reviewers: + rmistry@google.com
https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... File tools/perf/measurements/pixel_diff.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:12: class PixelDiff(legacy_page_test.LegacyPageTest): On 2017/06/07 20:15:11, wkorman wrote: > Add doc comment describing purpose. See power.py which has a brief one as an > example. Perhaps note file output/naming similar to my comment on the other > script. > > I also considered whether we should instead name this script 'screenshot' since > that's more self documenting. Could make a case either way. Let's see opinion of > our other reviewers. > > Note there is already as we've discussed in email a telemetry/util/screenshot.py > script so a benefit of naming as we have here is lack of ambiguity. Also, > perhaps we'd do more/different things for pixel diff down the line, so having > that specific naming is better. I agree that 'screenshot' might be a better name here. This measurement is not actually calculating any diffs, it's just outputting a screenshot, "pixel diff" will be the name used in CT because it will be the one actually doing the diff'ing. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:28: pass On 2017/06/07 20:32:12, nednguyen wrote: > On 2017/06/07 20:15:11, wkorman wrote: > > I see this is what rasterize_and_record_micro does as well. Two thoughts: > > > > (1) I would think we'd at least want to log something here. > > > > Two places where tools/perf logs timeouts currently for example: > > > > page_sets/system_health/multi_tab_stories.py > > contrib/cros_benchmarks/tab_switching_stories.py > > > > (2) seeking feedback from other reviewers: if we time out waiting for document > > ready, do we really still want to go ahead and try to take a screenshot? Or > > should we just report an error? Is it sufficient to just output results dict > > with 'saved_picture_count' of 0? How does Cluster Telemetry logic that calls > > into this script want to handle failure/error like this? Concern here is to > > ensure we (a) always check error results, and (b) make sure we do what we can > to > > make debugging in telemetry/cluster-telemetry logs easy for inevitable > failures. > > I think if we timeout here, it's better to report error or something like "I > can't process this" gracefully. Agreed, we should report a descriptive error. In CT whenever a measurement/benchmark reports an error it either retries it or moves on to the next webpage. Since CT deals with thousands of webpages, I tend to give up quickly and move on to the next page so that each run does not take too long. https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:32: os.makedirs(self._png_outdir) On 2017/06/07 20:15:11, wkorman wrote: > If we fail to makedir (and probably some other os calls here) it will raise an > exception. Is it ok for that case to propagate directly and cause script to just > fail w/ stack trace? See my question (2) above re: how CT wants to deal with > error handling. I think it's ok to propagate the exception in cases like this. If it cannot create the output dir there are problems going on that we want to know about. Would be useful to add logging saying "Creating xyz dir". https://codereview.chromium.org/2923163007/diff/1/tools/perf/measurements/pix... tools/perf/measurements/pixel_diff.py:39: screenshot = tab.Screenshot() On 2017/06/07 20:15:12, wkorman wrote: > This can raise a few exceptions, should we catch and handle in any manner? > Again, need to hear from other reviewers on (2) above. > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Hopefully the exception from Screenshot() will be descriptive. If it is not then we can look into making it more descriptive after we do a few test runs.
https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... File tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py (right): https://codereview.chromium.org/2923163007/diff/1/tools/perf/contrib/cluster_... tools/perf/contrib/cluster_telemetry/pixel_diff_ct.py:13: class PixelDiffCT(perf_benchmark.PerfBenchmark): On 2017/06/07 20:32:12, nednguyen wrote: > This should inherit s.t like a base CTBenchmark class so you have less boiler > plate to fill out Looks reasonable to me, but propose doing as separate patch as it's essentially a code maintenance/sharing cleanup work item. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot_ct.py (right): https://codereview.chromium.org/2923163007/diff/20001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_ct.py:15: # Screenshots written to local file with path-safe urls of pages as filenames. Move the comment text into the doc comment itself. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_ct.py:31: parser.add_option('--disable-javascript', action='store_true', Comment out this line and next two as well since otherwise people could see it with --help and think it is operational. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... File tools/perf/measurements/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... tools/perf/measurements/screenshot.py:50: screenshot = tab.Screenshot() Add logging.info before this call to note we're starting to take screenshot. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... tools/perf/measurements/screenshot.py:58: image_util.WritePngFile(screenshot, outpath) Add logging.info message before call to WritePngFile including output file path. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... tools/perf/measurements/screenshot.py:59: And one more logging.info after we wrote it to make clear that it was successful. https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... File tools/perf/measurements/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... tools/perf/measurements/screenshot_unittest.py:28: if results.failures: Looks like we still need to figure out some way to validate that we actually run a screenshot on linux vs hitting this return case?
https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... File tools/perf/measurements/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/20001/tools/perf/measurements... tools/perf/measurements/screenshot.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. Actually this measurement & the unittest files should be moved to tools/perf/contrib/.. since this is in the realm of CT benchmarks
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:21: # Javascript flag not yet implemented I suggest just leave this part out until someone implement them instead of commenting. https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:33: page.display_name) if this timeout, this should return instead of keep going https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:50: logging.info("Taking screenshot of %s", page.display_name) this log doesn't seem particularly useful IMO https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:64: if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time: Why we need this logic? If the pgn file are not written succesfully, it should throw exception & we don't even get to here. https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:67: results.current_page, 'saved_picture_count', 'count', is this saved_picture_count value used by Cluster Telemetry in anyway? https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:69: style nits: remove 3 blank lines
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:64: if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time: On 2017/06/08 20:18:25, nednguyen wrote: > Why we need this logic? If the pgn file are not written succesfully, it should > throw exception & we don't even get to here. I see. Do you want me to remove this and everything that relates to mtime then? https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:67: results.current_page, 'saved_picture_count', 'count', On 2017/06/08 20:18:26, nednguyen wrote: > is this saved_picture_count value used by Cluster Telemetry in anyway? Not sure if it will be used by CT - I was just following the example set by skpicture_printer. Should I remove for now?
https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:64: if os.path.exists(outpath) and os.path.getmtime(outpath) > last_mod_time: On 2017/06/08 20:38:34, lchoi wrote: > On 2017/06/08 20:18:25, nednguyen wrote: > > Why we need this logic? If the pgn file are not written succesfully, it should > > throw exception & we don't even get to here. > > I see. Do you want me to remove this and everything that relates to mtime then? sgtm https://codereview.chromium.org/2923163007/diff/40001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:67: results.current_page, 'saved_picture_count', 'count', On 2017/06/08 20:38:34, lchoi wrote: > On 2017/06/08 20:18:26, nednguyen wrote: > > is this saved_picture_count value used by Cluster Telemetry in anyway? > > Not sure if it will be used by CT - I was just following the example set by > skpicture_printer. Should I remove for now? Yeah. Code is liability, so it's usually better to not add them until we have concrete need :-)
https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:52: logging.info("Writing PNG file to %s", outpath) For nednguyen@: I asked Lawrence to add this and the logging below in an earlier patch set. The thought process was to ensure that if we run this with -v we get some useful output. Writing a png is an expensive operation and could take a while. We could see a hang in there. Normally info logging probably isn't even outputted AFAIK (if it is, switching these to debug log level would be fine). The thought was to be kind to our future selves by adding some useful debug logging in case it's helpful later. I just wanted to explain. If you still think this isn't useful given you are much more familiar with how these scripts are run, what issues crop up, etc. we can certainly remove it. https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:29: if results.failures and platform.system() == 'Linux': Don't we want the inverse? If it's Linux, we want to make sure they *are* supported?
https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/60001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:52: logging.info("Writing PNG file to %s", outpath) On 2017/06/08 23:14:41, wkorman wrote: > For nednguyen@: I asked Lawrence to add this and the logging below in an earlier > patch set. The thought process was to ensure that if we run this with -v we get > some useful output. Writing a png is an expensive operation and could take a > while. We could see a hang in there. Normally info logging probably isn't even > outputted AFAIK (if it is, switching these to debug log level would be fine). > > The thought was to be kind to our future selves by adding some useful debug > logging in case it's helpful later. > > I just wanted to explain. If you still think this isn't useful given you are > much more familiar with how these scripts are run, what issues crop up, etc. we > can certainly remove it. I see. If this take long time when you run it locally, then make sense to include it. Maybe making the log more explicit? logging.info("Writing PNG file to %s. This can take a long time..", outpath) start=time.time() .. logging.info("PNG file written successfully (took %s seconds)", time.time()-start)
lgtm Just two log level fixes, otherwise LGTM https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:29: logging.info("WaitForDocumentReadyStateToBeComplete() timeout, page: %s", This should be warning level (just replace info with warning) https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot.py:40: logging.info("Directory %s could not be created", self._png_outdir) warning level
lgtm with some nits. However, Ravis is the directory OWNER and you want to wait for his approval. https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:26: if platform.system() != 'Linux': You would want to do @decorators.Enabled('linux') instead. This will make the test run if the platform is Android. https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:36: self.assertTrue(os.access(path, os.R_OK)) In general, test should not leave files around after they are finished. We should clean up the png file path in a finally block: try: ... finally: remove directory that store png files
https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:36: self.assertTrue(os.access(path, os.R_OK)) On 2017/06/09 18:11:09, nednguyen wrote: > In general, test should not leave files around after they are finished. > We should clean up the png file path in a finally block: > try: > ... > finally: > remove directory that store png files I'm pretty sure the tearDown() fn above takes care of this. Just tried running a test and locating the test directory and .png based on the log output and I could not find them. But please correct me if I'm wrong!
https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... File tools/perf/contrib/cluster_telemetry/screenshot_unittest.py (right): https://codereview.chromium.org/2923163007/diff/80001/tools/perf/contrib/clus... tools/perf/contrib/cluster_telemetry/screenshot_unittest.py:36: self.assertTrue(os.access(path, os.R_OK)) On 2017/06/09 18:16:51, lchoi wrote: > On 2017/06/09 18:11:09, nednguyen wrote: > > In general, test should not leave files around after they are finished. > > We should clean up the png file path in a finally block: > > try: > > ... > > finally: > > remove directory that store png files > > I'm pretty sure the tearDown() fn above takes care of this. Just tried running a > test and locating the test directory and .png based on the log output and I > could not find them. But please correct me if I'm wrong! Ah right. Sorry that i missed the tearDown
lgtm
The CQ bit was checked by lchoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2923163007/#ps100001 (title: "Warning level logs, added decorators.enabled() to unit test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lchoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, rmistry@google.com, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2923163007/#ps120001 (title: "Unit test debug")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lchoi@chromium.org
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lchoi@chromium.org
Description was changed from ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. BUG=536285 ========== to ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. ==========
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by lchoi@chromium.org
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lchoi@chromium.org
The CQ bit was checked by lchoi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lchoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, rmistry@google.com, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2923163007/#ps310001 (title: "Addex pixel assert to unit test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 310001, "attempt_start_ts": 1497486148221840, "parent_rev": "0e544c0ef104b568910a2f00348350da9e7e953d", "commit_rev": "78f73931528e91c22e91f4b3b8f830c6ed86c9db"}
Message was sent while issue was closed.
Description was changed from ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. ========== to ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. Review-Url: https://codereview.chromium.org/2923163007 Cr-Commit-Position: refs/heads/master@{#479558} Committed: https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f8... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f8...
Message was sent while issue was closed.
Description was changed from ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. Review-Url: https://codereview.chromium.org/2923163007 Cr-Commit-Position: refs/heads/master@{#479558} Committed: https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f8... ========== to ========== Implemented telemetry benchmark that loads page and outputs screenshot. Logic for '--disable-javascript' flag not implemented yet. BUG=536285 Review-Url: https://codereview.chromium.org/2923163007 Cr-Commit-Position: refs/heads/master@{#479558} Committed: https://chromium.googlesource.com/chromium/src/+/78f73931528e91c22e91f4b3b8f8... ========== |