Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406593002:
Add script to process results from Android render tests. (Closed)
|
Created:
4 years, 2 months ago by mikecase (-- gone --) Modified:
3 years, 11 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd script to process results from Android render tests.
This script grabs the render test results from the device, uploads
the rendered images to Google Storage, and generates a webpage
showing the results.
Review-Url: https://codereview.chromium.org/2406593002
Cr-Commit-Position: refs/heads/master@{#442408}
Committed: https://chromium.googlesource.com/chromium/src/+/6f52f71437d12c64d03280ad7f528dbca9b321b4
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed some useless mdl classes #
Total comments: 17
Patch Set 3 : Made uploading faster (parallel). Made webpage use mdl-grid and it's pretty. Addressed some comment… #Patch Set 4 : Add script to process results from Android render tests. #
Total comments: 22
Patch Set 5 : John's comments #
Total comments: 4
Patch Set 6 : Add script to process results from Android render tests. #Patch Set 7 : Made script fail kinda gracefully if PIL fails to import #
Total comments: 3
Patch Set 8 : Add script to process results from Android render tests. #Patch Set 9 : Add script to process results from Android render tests. #Patch Set 10 : Add script to process results from Android render tests. #
Messages
Total messages: 40 (10 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org
You mentioned this yesterday. What do you think of things like this. This script pulls the render test results from a specified dir on the device, uploads the results to a GS bucket, and then outputs the results in simple webpage. Would have the bots just run this script after the render instrumentation tests, and link the webpage so devs could see results easily.
mikecase@chromium.org changed reviewers: + yolandyan@chromium.org
+ yoland for advice on making a semi-pretty webpage for the results. Could you take a look at the html.jinja2 file please :D https://codereview.chromium.org/2406593002/diff/1/build/android/render_tests/... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/1/build/android/render_tests/... build/android/render_tests/process_render_test_results.py:43: """Upload image to the render tests GS bucket.""" Add doc about Return value here. (its just a url to link to in the results webpage)
mikecase@chromium.org changed reviewers: + peconn@chromium.org
+ peconn I wanted to possibly use the RenderUtils.java stuff for some WebView tests. John mentioned that you wanted the results to be more easily displayed on the Chromium waterfall. Here is a CL for a script a bot could run to generate a webpage showing all of the render test results. please take a look and comment :D
This looks really good, thanks!
Part of the usecase for this is to allow the dev to download the failed renders
so they can paste them into the repository overriding the golden files. (The dev
can then upload the failed images as the new goldens if the changes were all
intentional).
Would it be possible to add a link where you can download a zip file of all the
failures.
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te...
File build/android/render_tests/process_render_test_results.py (right):
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te...
build/android/render_tests/process_render_test_results.py:146: help='Path on
device to look for render test images')
At the moment all the render results go in
'chrome/test/data/android/render_tests/' with the failures in '.../failures'. In
the future they may go in a tree based there, eg:
chrome/test/data/android/render_tests/
chrome/test/data/android/render_tests/new_tab_page/
chrome/test/data/android/render_tests/signin/
chrome/test/data/android/render_tests/failures/
chrome/test/data/android/render_tests/new_tab_page/failures/
chrome/test/data/android/render_tests/signin/failures/
What you're doing is great - just to keep that in mind for the future.
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te...
File build/android/render_tests/render_webpage.html.jinja2 (right):
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te...
build/android/render_tests/render_webpage.html.jinja2:33: <td><img src="{{
result['golden_image'] }}" width="100%"></td>
The images could be quite large, maybe it would be worth scaling them so they
can be viewed without having to scroll horizontally, then giving full size on
click.
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:43: """Upload image to the render tests GS bucket. Would it be better to have recipe control the name of the bucket? https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:61: diff_image = diff_image.convert('L') nit: maybe just ``` return ImageChops.difference(failure_image, golden_image) .convert('L') .point(lambd i: 255 i else 0) ``` and change the method if the diff compute algo changes in the future https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:94: golden_file = os.path.join(temp_dir, failure_filename) hmm, out of the curiosity, when is the golden images pushed to devices and where were they stored initially? https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:116: if test_class not in results_dict: you can use collections.defaultdict here results_dict = defaultdict(lambda: defaultdict(list)) which return results_dict[x] as a defaultdict for which the default return is a list then again, readability -> 0 lol
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:62: diff_image = diff_image.point(lambda i: 255 if i else 0) ImageMagick can do a really nice diff where you've got one of the original images faint in the background, eg: http://i.stack.imgur.com/hKAXB.png That'd probably be easy to implement as putting diff_image on top of the faded golden image. https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:94: golden_file = os.path.join(temp_dir, failure_filename) On 2016/10/13 22:03:38, the real yoland wrote: > hmm, out of the curiosity, when is the golden images pushed to devices and where > were they stored initially? The goldens are stored in src/chrome/test/data/android/render_tests and are pushed before the instrumentation tests are run.
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:94: golden_file = os.path.join(temp_dir, failure_filename) On 2016/10/14 at 09:08:35, PEConn wrote: > On 2016/10/13 22:03:38, the real yoland wrote: > > hmm, out of the curiosity, when is the golden images pushed to devices and where > > were they stored initially? > > The goldens are stored in src/chrome/test/data/android/render_tests and are pushed before the instrumentation tests are run. +peconn Not sure how the entire of the process works, but I assume it's easier to just store the goldens in a bucket directory and fetch them from there accordingly to compare without getting device involved? the push and pull seems redundant. But then maybe that somehow would limit the flexibility of creating new type of rendering tests?
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:116: if test_class not in results_dict: On 2016/10/13 at 22:03:38, the real yoland wrote: > you can use collections.defaultdict here > results_dict = defaultdict(lambda: defaultdict(list)) > which return results_dict[x] as a defaultdict for which the default return is a list > > then again, readability -> 0 lol you have to change it back to dict at the end though `dict(results_dict)` or else anything jinja template refers to in the default_dict that wasn't supposed to exist will return with an empty list, which would make bug hard to catch
On 2016/10/14 at 18:37:37, yolandyan wrote: > https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... > File build/android/render_tests/process_render_test_results.py (right): > > https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... > build/android/render_tests/process_render_test_results.py:94: golden_file = os.path.join(temp_dir, failure_filename) > On 2016/10/14 at 09:08:35, PEConn wrote: > > On 2016/10/13 22:03:38, the real yoland wrote: > > > hmm, out of the curiosity, when is the golden images pushed to devices and where > > > were they stored initially? > > > > The goldens are stored in src/chrome/test/data/android/render_tests and are pushed before the instrumentation tests are run. > > +peconn > > Not sure how the entire of the process works, but I assume it's easier to just store the goldens in a bucket directory and fetch them from there accordingly to compare without getting device involved? the push and pull seems redundant. > > But then maybe that somehow would limit the flexibility of creating new type of rendering tests? The goldens are checked into Chromium. So we probably dont want them stored in a google storage bucket. However, I could grab the goldens from the checkout and only pull the failures from the device. This would make this script probably only a tiny bit faster though Im guessing, but I can look into doing this.
On 2016/10/14 19:01:19, mikecase wrote: > On 2016/10/14 at 18:37:37, yolandyan wrote: > > > https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... > > File build/android/render_tests/process_render_test_results.py (right): > > > > > https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... > > build/android/render_tests/process_render_test_results.py:94: golden_file = > os.path.join(temp_dir, failure_filename) > > On 2016/10/14 at 09:08:35, PEConn wrote: > > > On 2016/10/13 22:03:38, the real yoland wrote: > > > > hmm, out of the curiosity, when is the golden images pushed to devices and > where > > > > were they stored initially? > > > > > > The goldens are stored in src/chrome/test/data/android/render_tests and are > pushed before the instrumentation tests are run. > > > > +peconn > > > > Not sure how the entire of the process works, but I assume it's easier to just > store the goldens in a bucket directory and fetch them from there accordingly to > compare without getting device involved? the push and pull seems redundant. > > > > But then maybe that somehow would limit the flexibility of creating new type > of rendering tests? > > The goldens are checked into Chromium. So we probably dont want them stored in a > google storage bucket. However, I could grab the goldens from the checkout and > only pull the failures from the device. This would make this script probably > only a tiny bit faster though Im guessing, but I can look into doing this. I think pulling both the goldens and the failures from the device will make this simpler, so I'm happy leaving it like that until we know how much time it wastes.
https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:61: diff_image = diff_image.convert('L') On 2016/10/13 at 22:03:38, the real yoland wrote: > nit: maybe just > ``` > return ImageChops.difference(failure_image, golden_image) > .convert('L') > .point(lambd i: 255 i else 0) > ``` > and change the method if the diff compute algo changes in the future Done https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:62: diff_image = diff_image.point(lambda i: 255 if i else 0) On 2016/10/14 at 09:08:35, PEConn wrote: > ImageMagick can do a really nice diff where you've got one of the original images faint in the background, eg: http://i.stack.imgur.com/hKAXB.png > > That'd probably be easy to implement as putting diff_image on top of the faded golden image. I've never used ImageMagick. Is it available in Chromium? https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:116: if test_class not in results_dict: On 2016/10/14 at 18:59:25, the real yoland wrote: > On 2016/10/13 at 22:03:38, the real yoland wrote: > > you can use collections.defaultdict here > > results_dict = defaultdict(lambda: defaultdict(list)) > > which return results_dict[x] as a defaultdict for which the default return is a list > > > > then again, readability -> 0 lol > > you have to change it back to dict at the end though `dict(results_dict)` or else anything jinja template refers to in the default_dict that wasn't supposed to exist will return with an empty list, which would make bug hard to catch Done https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:146: help='Path on device to look for render test images') On 2016/10/11 at 12:20:07, PEConn wrote: > At the moment all the render results go in 'chrome/test/data/android/render_tests/' with the failures in '.../failures'. In the future they may go in a tree based there, eg: > > chrome/test/data/android/render_tests/ > chrome/test/data/android/render_tests/new_tab_page/ > chrome/test/data/android/render_tests/signin/ > > chrome/test/data/android/render_tests/failures/ > chrome/test/data/android/render_tests/new_tab_page/failures/ > chrome/test/data/android/render_tests/signin/failures/ > > What you're doing is great - just to keep that in mind for the future. Noted https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/render_webpage.html.jinja2 (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/render_webpage.html.jinja2:33: <td><img src="{{ result['golden_image'] }}" width="100%"></td> On 2016/10/11 at 12:20:07, PEConn wrote: > The images could be quite large, maybe it would be worth scaling them so they can be viewed without having to scroll horizontally, then giving full size on click. Made clicking them open them in a new tab. The images are big still if you fullscreen the webpage, but it looks pretty good. Can easily fiddle with this later. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:127: image_files['goldens'].append(golden_file) I dont upload the file immediately since it is much much faster to upload them all at once in parallel using "gsutil -m cp" command.
LGTM - my comments at this point are just suggestions. https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:62: diff_image = diff_image.point(lambda i: 255 if i else 0) On 2016/10/18 16:00:12, mikecase wrote: > On 2016/10/14 at 09:08:35, PEConn wrote: > > ImageMagick can do a really nice diff where you've got one of the original > images faint in the background, eg: http://i.stack.imgur.com/hKAXB.png > > > > That'd probably be easy to implement as putting diff_image on top of the faded > golden image. > > I've never used ImageMagick. Is it available in Chromium? ImageMagick is a command line tool (it may be already installed if you have a Linux machine). I wasn't suggesting that we use it, but that we create diff images in a similar style to it. https://codereview.chromium.org/2406593002/diff/20001/build/android/render_te... build/android/render_tests/process_render_test_results.py:94: golden_file = os.path.join(temp_dir, failure_filename) On 2016/10/14 18:37:37, the real yoland wrote: > On 2016/10/14 at 09:08:35, PEConn wrote: > > On 2016/10/13 22:03:38, the real yoland wrote: > > > hmm, out of the curiosity, when is the golden images pushed to devices and > where > > > were they stored initially? > > > > The goldens are stored in src/chrome/test/data/android/render_tests and are > pushed before the instrumentation tests are run. > > +peconn > > Not sure how the entire of the process works, but I assume it's easier to just > store the goldens in a bucket directory and fetch them from there accordingly to > compare without getting device involved? the push and pull seems redundant. > > But then maybe that somehow would limit the flexibility of creating new type of > rendering tests? I can't think of how it would reduce the flexibility at the moment, grabbing the goldens from src/ should work fine.
https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:18: from collections import defaultdict nit: import collections, then use collections.defaultdict. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:26: sys.path.append(os.path.abspath( nit: This doesn't need to be abspath, DIR_SOURCE_ROOT is already absolute. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:35: r'(?P<test_class>\w+)\.(?P<description>\w+)\.' super nit: I would split this s.t. each capture is on its own line. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:44: _CHROMIUM_SRC = os.path.join( This is host_paths.DIR_SOURCE_ROOT. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:51: cmd = [download_from_google_storage.GSUTIL_DEFAULT_PATH, '-m', 'cp'] using "download_from_google_storage" to upload files... 🤔 Why not just os.path.join(find_depot_tools.DEPOT_TOOLS_PATH, 'gsutil.py') ? https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:88: image_files = { Why aren't these just three separate lists? https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:102: device.adb.Pull(failures_device_dir, temp_dir) device.PullFile plz https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:106: if not m: This should log something if we hit an unexpected file name. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:157: with open(html_file, "wb") as f: nit: single quotes https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:160: if temp_dir: we really ought to extract https://codesearch.chromium.org/chromium/src/build/android/gyp/util/build_uti... into somewhere more generally usable.
https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:18: from collections import defaultdict On 2016/10/19 at 16:11:32, jbudorick wrote: > nit: import collections, then use collections.defaultdict. Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:26: sys.path.append(os.path.abspath( On 2016/10/19 at 16:11:32, jbudorick wrote: > nit: This doesn't need to be abspath, DIR_SOURCE_ROOT is already absolute. Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:35: r'(?P<test_class>\w+)\.(?P<description>\w+)\.' On 2016/10/19 at 16:11:32, jbudorick wrote: > super nit: I would split this s.t. each capture is on its own line. Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:44: _CHROMIUM_SRC = os.path.join( On 2016/10/19 at 16:11:32, jbudorick wrote: > This is host_paths.DIR_SOURCE_ROOT. Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:51: cmd = [download_from_google_storage.GSUTIL_DEFAULT_PATH, '-m', 'cp'] On 2016/10/19 at 16:11:33, jbudorick wrote: > using "download_from_google_storage" to upload files... 🤔 > > Why not just os.path.join(find_depot_tools.DEPOT_TOOLS_PATH, 'gsutil.py') ? I thought it was silly too. upload_to_google_storage imports this constant from download_from_google_storage too. https://cs.chromium.org/chromium/tools/depot_tools/upload_to_google_storage.p... I can change it to suggestion though. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:88: image_files = { On 2016/10/19 at 16:11:33, jbudorick wrote: > Why aren't these just three separate lists? Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:102: device.adb.Pull(failures_device_dir, temp_dir) On 2016/10/19 at 16:11:32, jbudorick wrote: > device.PullFile plz Hmmm, didn't know this would work. Name and docstring suggested this function only works on single File whereas I want to pull directory. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:106: if not m: On 2016/10/19 at 16:11:33, jbudorick wrote: > This should log something if we hit an unexpected file name. Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:139: failures_zipfile = os.path.join(temp_dir, 'failures.zip') Added check so that no zip is created/uploaded if there are no failures. https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:157: with open(html_file, "wb") as f: On 2016/10/19 at 16:11:33, jbudorick wrote: > nit: single quotes Done https://codereview.chromium.org/2406593002/diff/60001/build/android/render_te... build/android/render_tests/process_render_test_results.py:160: if temp_dir: On 2016/10/19 at 16:11:32, jbudorick wrote: > we really ought to extract https://codesearch.chromium.org/chromium/src/build/android/gyp/util/build_uti... into somewhere more generally usable. Ack
Sorry about the delay. lgtm w/ q https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... build/android/render_tests/process_render_test_results.py:148: _UploadFiles(upload_dir, [failures_zipfile]) Why are we uploading the failures images both zipped and unzipped? https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... build/android/render_tests/process_render_test_results.py:156: # pylint: disable=no-member ?
https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... build/android/render_tests/process_render_test_results.py:148: _UploadFiles(upload_dir, [failures_zipfile]) On 2016/11/07 at 01:44:45, jbudorick wrote: > Why are we uploading the failures images both zipped and unzipped? Because, I want to display the images on the generated html page (which I will link from the bot results). And I want to let devs download the failures zip if they want to update the golden images. https://codereview.chromium.org/2406593002/diff/80001/build/android/render_te... build/android/render_tests/process_render_test_results.py:156: # pylint: disable=no-member On 2016/11/07 at 01:44:45, jbudorick wrote: > ? Getting this error... E:157,32: Instance of 'str' has no 'render' member (no-member) ...which seems to be a pylint bug.... https://github.com/PyCQA/pylint/issues/490
sgtm / still lgtm
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org Link to the patchset: https://codereview.chromium.org/2406593002/#ps100001 (title: "Add script to process results from Android render tests.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/11/07 at 20:19:16, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) waaah Presubmit failure due to PIL module not found on this presubmit bot. PIL module is on the Android test bot I had checked. Not really sure why one bot has it and another does not. Going to add #pylint ignore for this Just double checked... >>> help('modules') Please wait a moment while I gather a list of all available modules... .... OpenSSL PIL PSDraw .... PIL is there on the Android Test (trial)(dbg) bot on the chromium.fyi waterfall.
On 2016/11/07 21:51:53, mikecase wrote: > On 2016/11/07 at 20:19:16, commit-bot wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > waaah > Presubmit failure due to PIL module not found on this presubmit bot. PIL module > is on the Android test bot I had checked. Not really sure why one bot has it and > another does not. Going to add #pylint ignore for this > > > Just double checked... > > >>> help('modules') > Please wait a moment while I gather a list of all available modules... > .... > OpenSSL > PIL > PSDraw > .... > > PIL is there on the Android Test (trial)(dbg) bot on the chromium.fyi waterfall. # pylint: disable=import-error ?
also, we'll likely want to gracefully handle the case where we can't import PIL.
On 2016/11/07 at 21:54:51, jbudorick wrote: > also, we'll likely want to gracefully handle the case where we can't import PIL. Moved the PIL inside the 1 function that they are used. If it fails to import, the diff files wont be generated and I log an error. The end result will be that broken images will appear on generated html page for the diff images.
https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... build/android/render_tests/process_render_test_results.py:134: diff_images.append(diff_file) Should this still be populated if we fail to create a diff? https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... build/android/render_tests/process_render_test_results.py:143: 'diff_image': _GoogleStorageUrl(diff_upload_dir, diff_file), Same.
https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... File build/android/render_tests/process_render_test_results.py (right): https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... build/android/render_tests/process_render_test_results.py:134: diff_images.append(diff_file) On 2016/11/07 at 22:49:08, jbudorick wrote: > Should this still be populated if we fail to create a diff? Changed so that now if PIL is not available, we won't even try to create diff, upload them, and wont display them at all in the html page.
On 2016/11/10 22:43:03, mikecase wrote: > https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... > File build/android/render_tests/process_render_test_results.py (right): > > https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... > build/android/render_tests/process_render_test_results.py:134: > diff_images.append(diff_file) > On 2016/11/07 at 22:49:08, jbudorick wrote: > > Should this still be populated if we fail to create a diff? > > Changed so that now if PIL is not available, we won't even try to create diff, > upload them, and wont display them at all in the html page. I'm assuming this is blocked on https://codereview.chromium.org/2439213002/ ?
On 2016/11/21 at 17:57:08, peconn wrote: > On 2016/11/10 22:43:03, mikecase wrote: > > https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... > > File build/android/render_tests/process_render_test_results.py (right): > > > > https://codereview.chromium.org/2406593002/diff/120001/build/android/render_t... > > build/android/render_tests/process_render_test_results.py:134: > > diff_images.append(diff_file) > > On 2016/11/07 at 22:49:08, jbudorick wrote: > > > Should this still be populated if we fail to create a diff? > > > > Changed so that now if PIL is not available, we won't even try to create diff, > > upload them, and wont display them at all in the html page. > > I'm assuming this is blocked on https://codereview.chromium.org/2439213002/ ? Yeah. I could commit this, but it won't do anything without other CL getting lgtm and landing.
rebased and committing this. CL to add this to recipes got approved.
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2406593002/#ps180001 (title: "Add script to process results from Android render tests.")
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": 180001, "attempt_start_ts": 1484000144299770,
"parent_rev": "ab423c7eed625861e758a9dbd1e26c9650fb9347", "commit_rev":
"6f52f71437d12c64d03280ad7f528dbca9b321b4"}
Message was sent while issue was closed.
Description was changed from ========== Add script to process results from Android render tests. This script grabs the render test results from the device, uploads the rendered images to Google Storage, and generates a webpage showing the results. ========== to ========== Add script to process results from Android render tests. This script grabs the render test results from the device, uploads the rendered images to Google Storage, and generates a webpage showing the results. Review-Url: https://codereview.chromium.org/2406593002 Cr-Commit-Position: refs/heads/master@{#442408} Committed: https://chromium.googlesource.com/chromium/src/+/6f52f71437d12c64d03280ad7f52... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/6f52f71437d12c64d03280ad7f52... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
