[webkit-reviews] review granted: [Bug 43519] Upload incremental webkit test results json to server : [Attachment 63510] Proposed Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Victor Wang <victorw at chromium.org>'s
request for review:
Bug 43519: Upload incremental webkit test results json to server
https://bugs.webkit.org/show_bug.cgi?id=43519

Attachment 63510: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=63510&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Looks good. Please make the changes below and feel free to commit.

> +	   # Generate the JSON output file that has full results.
>	   if not self._json:
>	       self._json = self.get_json()
>	   if self._json:
> -	       # Specify separators in order to get compact encoding.
> -	       json_data = simplejson.dumps(self._json, separators=(',', ':'))
> -	       json_string = self.JSON_PREFIX + json_data + self.JSON_SUFFIX
> -
> -	       results_file = codecs.open(self._results_file_path, "w",
"utf-8")
> -	       results_file.write(json_string)
> -	       results_file.close()
> +	       self._generate_json_file(self._json, self._results_file_path)
> +
> +	   # Generate the JSON output file that only has incremental results.
> +	   if self._generate_incremental_results:
> +	       json = self.get_json(incremental=True)
> +	       if json:
> +		   self._generate_json_file(
> +		       json, self._incremental_results_file_path)

Once we get all the bots using incremental results, I think we should have
setting _generate_incremental_results mean that we don't write out the full
results file. Mind putting in a FIXME for that?

> +	       for d in buildinfo[JRG.FIXABLE]:
> +		   for (k, v) in d.iteritems():
> +		       if k in fixable:
> +			   fixable[k] = fixable[k] + v
> +		       else:
> +			   fixable[k] = v

> +		   for r in test[JRG.RESULTS]:
> +		       if r[1] == JRG.FAIL_RESULT:
> +			   failed = r[0]
> +		   self.assertTrue(failed == 1)
> +
> +		   timing = 0
> +		   for t in test[JRG.TIMES]:
> +		       if t[1] == test_timings[test_name]:
> +			   timing = t[0]
> +		   self.assertTrue(timing == 1)

For readability sake, abbreviated variable names are discouraged in WebKit code
(i.e. for d, k, v, r and t).