Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20654006:
download and rebaseline images using server (Closed)
|
Created:
7 years, 4 months ago by Zach Reizner Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptiondownload and rebaseline images using server
BUG=
R=epoger@google.com
Committed: https://code.google.com/p/skia/source/detail?r=10607
Patch Set 1 #
Total comments: 31
Patch Set 2 : refactor #Patch Set 3 : actually saving the file edition #
Total comments: 9
Patch Set 4 : better comments #Patch Set 5 : remove file_name #Patch Set 6 : fix other missing filepath #
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:137: expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS][0][1] = hash_value Curse you 80 character line length.
I'll defer to elliot on this one.
https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py File tools/jsondiff.py (right): https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py#newcode153 tools/jsondiff.py:153: old_results = self._GetExpectedResults(self._GetFileContentsAsString(oldfile)) I think it would be a bit cleaner to have this method call GenerateDiffDictFromStrings(), like so: return self.GenerateDiffDictFromStrings( oldjson=self._GetFileContentsAsString(oldfile), newjson=self._GetFileContentsAsString(newfile) ) That way, the logic around what-to-compare-against-if-newfile-is-None can be in exactly one place, rather than copy-pasted into 2 places. To do this, you'll need to update _GetFileContentsAsString() to return None if filepath=None, which seems reasonable. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:31: // Make it so the maximum size of an image is 400, and the images are scaled Please create a constant, near the top of the file, to hold this value. Otherwise, the documentation and the reality WILL diverge. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:105: // reapplied later. Is this hosted somewhere that I can look at it, and see what the effect looks like? https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:108: var flashDuration = success ? 500 : 800; I take it this is in milliseconds? If so, please rename as flashDurationMillis to make it clear. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:137: expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS][0][1] = hash_value On 2013/08/01 22:24:40, Zach Reizner wrote: > Curse you 80 character line length. Indeed. If you want to, you can split it like so (I think): expected_results[image_name]\ [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS]\ [0][1] = hash_value https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:143: 'bitmap-64bitMD5', please use the constant reference to this string value, from gm_json. Also, please add a TODO at the top of this function indicating that it is hard-coded to work only with this particular hashType. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:150: def get_git_version(path): IMHO, get_head_version or get_base_version would be a better name. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:158: # git-show will only work with paths relative to the root of the git repo. This is great, but FYI, git-show works with a path relative to current working dir also. You just have to start the path with ./ : git show HEAD:./path/to/file See https://code.google.com/p/skia/source/browse/trunk/tools/svndiff.py?r=10288#205 https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:179: Normally this matches --expectations-filename-output for the I don't understand why we need the updated expected-results.json to be in a separate file anymore, now that we use "git show" to see the local modifications to expected-results.json. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:183: self.expectations_dir = expectations_dir Typically, we would use underscored (self._expectations_dir) to indicate that these are private variables. (We only want them to be accessed from within this class, right?) See http://stackoverflow.com/questions/1301346/the-meaning-of-a-single-and-a-doub... https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:191: changed images and comparing them skpdiff. I think it would be clearer in bullet list form: Generate all the data needed to compare GMs: - download the expected and actual versions of failing images - run skpdiff over those image pairs https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:215: def download_changed_images(self, expected_image_dir, actual_image_dir): Ultimately, it would be good for us to have self-tests for tools like this. And those self-tests will be easier to write if we separate out the various parts of this method: 1. given a directory, find all the matched sets of JSON files of interest 2. for each matched set of JSON files, get a list of images (as imagename/checksum pairs) to download 3. download a list of images to desired directory https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:221: @param expected_image_dir The directory to place downloaded expected maybe clearer to call this "The directory to download expected images into", and similar for actual below.
https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py File tools/jsondiff.py (right): https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py#newcode128 tools/jsondiff.py:128: test_name, filepath, digest_pair[0])) Just a quick thing my linter caught: filepath doesn't exist. I'm not sure how you'd like to fix this, so I haven't done anything to it. https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py#newcode153 tools/jsondiff.py:153: old_results = self._GetExpectedResults(self._GetFileContentsAsString(oldfile)) On 2013/08/02 14:33:06, epoger wrote: > I think it would be a bit cleaner to have this method call > GenerateDiffDictFromStrings(), like so: > > return self.GenerateDiffDictFromStrings( > oldjson=self._GetFileContentsAsString(oldfile), > newjson=self._GetFileContentsAsString(newfile) > ) > > That way, the logic around what-to-compare-against-if-newfile-is-None can be in > exactly one place, rather than copy-pasted into 2 places. > > To do this, you'll need to update _GetFileContentsAsString() to return None if > filepath=None, which seems reasonable. Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:31: // Make it so the maximum size of an image is 400, and the images are scaled On 2013/08/02 14:33:06, epoger wrote: > Please create a constant, near the top of the file, to hold this value. > Otherwise, the documentation and the reality WILL diverge. Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:105: // reapplied later. On 2013/08/02 14:33:06, epoger wrote: > Is this hosted somewhere that I can look at it, and see what the effect looks > like? Nope. Never done that before on the corp network. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:108: var flashDuration = success ? 500 : 800; On 2013/08/02 14:33:06, epoger wrote: > I take it this is in milliseconds? If so, please rename as flashDurationMillis > to make it clear. Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:137: expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS][0][1] = hash_value On 2013/08/02 14:33:06, epoger wrote: > On 2013/08/01 22:24:40, Zach Reizner wrote: > > Curse you 80 character line length. > > Indeed. > > If you want to, you can split it like so (I think): > > expected_results[image_name]\ > [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS]\ > [0][1] = hash_value I do not want to. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:143: 'bitmap-64bitMD5', On 2013/08/02 14:33:06, epoger wrote: > please use the constant reference to this string value, from gm_json. > > Also, please add a TODO at the top of this function indicating that it is > hard-coded to work only with this particular hashType. Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:150: def get_git_version(path): On 2013/08/02 14:33:06, epoger wrote: > IMHO, get_head_version or get_base_version would be a better name. Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:158: # git-show will only work with paths relative to the root of the git repo. On 2013/08/02 14:33:06, epoger wrote: > This is great, but FYI, git-show works with a path relative to current working > dir also. You just have to start the path with ./ : > > git show HEAD:./path/to/file > > See > https://code.google.com/p/skia/source/browse/trunk/tools/svndiff.py?r=10288#205 Thanks https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:179: Normally this matches --expectations-filename-output for the On 2013/08/02 14:33:06, epoger wrote: > I don't understand why we need the updated expected-results.json to be in a > separate file anymore, now that we use "git show" to see the local modifications > to expected-results.json. You make an excellent point. Derek and I have been discussing ways of improving the interface. A big part of that is making it clear what the user has rebaselined, even if the server is restarted. This may mean comparing updated-results.json with the HEAD and workign tree versions of expected-results.json. It will be the next CL, so I'd like to keep it this way for now. Also, this way rebaselining doesn't get checked into the tree until the user explicitly chooses to. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:183: self.expectations_dir = expectations_dir On 2013/08/02 14:33:06, epoger wrote: > Typically, we would use underscored (self._expectations_dir) to indicate that > these are private variables. (We only want them to be accessed from within this > class, right?) > > See > http://stackoverflow.com/questions/1301346/the-meaning-of-a-single-and-a-doub... Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:191: changed images and comparing them skpdiff. On 2013/08/02 14:33:06, epoger wrote: > I think it would be clearer in bullet list form: > > Generate all the data needed to compare GMs: > - download the expected and actual versions of failing images > - run skpdiff over those image pairs Done. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:221: @param expected_image_dir The directory to place downloaded expected On 2013/08/02 14:33:06, epoger wrote: > maybe clearer to call this "The directory to download expected images into", and > similar for actual below. Done.
Cool, we're pretty close on this... https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py File tools/jsondiff.py (right): https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py#newcode128 tools/jsondiff.py:128: test_name, filepath, digest_pair[0])) On 2013/08/02 21:30:58, Zach Reizner wrote: > Just a quick thing my linter caught: filepath doesn't exist. I'm not sure how > you'd like to fix this, so I haven't done anything to it. Ah. That's because, before this CL, filepath was the argument to this function (see https://codereview.chromium.org/20654006/diff/12001/tools/jsondiff.py ). I guess just remove the "file %s" and filepath parts. Another vote in favor of self-tests that exercise as many cases as possible. :-) https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/diff_viewer.js#... tools/skpdiff/diff_viewer.js:105: // reapplied later. On 2013/08/02 21:30:58, Zach Reizner wrote: > On 2013/08/02 14:33:06, epoger wrote: > > Is this hosted somewhere that I can look at it, and see what the effect looks > > like? > > Nope. Never done that before on the corp network. It's easy to do. See the "User directories" section within https://support.google.com/techstop/answer/3222246 . Basically, you just copy files into /google/data/rw/users/za/zachr/www and then they are viewable within https://x20web.corp.google.com/~zachr/ . It may or may not make sense for you to bother setting this up, depending on how much time you have left (not much, at least for now!) But it's an easy way to let your reviewers try out a web service. https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:137: expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS][0][1] = hash_value On 2013/08/02 21:30:58, Zach Reizner wrote: > On 2013/08/02 14:33:06, epoger wrote: > > On 2013/08/01 22:24:40, Zach Reizner wrote: > > > Curse you 80 character line length. > > > > Indeed. > > > > If you want to, you can split it like so (I think): > > > > expected_results[image_name]\ > > [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS]\ > > [0][1] = hash_value > > I do not want to. Fine, be that way. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:171: class GMInstance: Information about a particular GM test result: device_name = which device type it was rendered on image_name = which GM test and config expected_hash = expected hash value actual_hash = actual hash value https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:235: """Build an array of expectations that have changed with the actual Maybe this would be clearer? Fill self._expectations array with a GMInstance object for each test whose expectation is different in these two files: 1. the locally modified version of updated_results.json 2. the "git head" version of expected_results.json. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:271: # Download each image that had a differing result. We take care Not downloading anymore. Instead, maybe... Store old/new checksums of each updated image. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:282: """Download the expected and actual iamges for the _expectations array. iamges -> images https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:301: print('Downloading', expectation.image_name, A Python partisan would suggest: print 'Downloading %s for device %s' % ( expectation.image_name, expectation.device_name)
https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py File tools/jsondiff.py (right): https://codereview.chromium.org/20654006/diff/1/tools/jsondiff.py#newcode128 tools/jsondiff.py:128: test_name, filepath, digest_pair[0])) On 2013/08/06 15:26:43, epoger wrote: > Another vote in favor of self-tests that exercise as many cases as possible. :-) P.S. I'm not suggesting you take the time to add self-tests for this right now. Just saying this is an example of why they are generally useful.
https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:171: class GMInstance: On 2013/08/06 15:26:43, epoger wrote: > Information about a particular GM test result: > device_name = which device type it was rendered on > image_name = which GM test and config > expected_hash = expected hash value > actual_hash = actual hash value Done. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:235: """Build an array of expectations that have changed with the actual On 2013/08/06 15:26:43, epoger wrote: > Maybe this would be clearer? > > Fill self._expectations array with a GMInstance object for each test whose > expectation is different in these two files: > 1. the locally modified version of updated_results.json > 2. the "git head" version of expected_results.json. Done. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:271: # Download each image that had a differing result. We take care On 2013/08/06 15:26:43, epoger wrote: > Not downloading anymore. Instead, maybe... > > Store old/new checksums of each updated image. Done. https://codereview.chromium.org/20654006/diff/12001/tools/skpdiff/skpdiff_ser... tools/skpdiff/skpdiff_server.py:282: """Download the expected and actual iamges for the _expectations array. On 2013/08/06 15:26:43, epoger wrote: > iamges -> images Done.
lgtm
Message was sent while issue was closed.
Committed patchset #6 manually as r10607 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
