[webkit-reviews] review denied: [Bug 38599] Merge incremental results.json on test results app engine server : [Attachment 56280] Updated patch per Eric's comments.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Victor Wang <victorw at chromium.org>'s
request for review:
Bug 38599: Merge incremental results.json on test results app engine server
https://bugs.webkit.org/show_bug.cgi?id=38599

Attachment 56280: Updated patch per Eric's comments.
https://bugs.webkit.org/attachment.cgi?id=56280&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
All nits except for one correctness concern below. Thanks for waiting patiently
for this review. Also, thanks for writing tests. Makes it much easier to be
confident about the correctness of the merging code. Please fix the below and
upload a new patch.

> +++ WebKitTools/TestResultServer/model/datastorefile.py	(revision 0)
There should be a trailing newline at the end of the file.

> +	   # Tests properties are merged in _merg_tests.
Typo: _merg_tests

> +	       # Return if not all build numbers in the incremental json
results
> +	       # are newer than the most recent build in the aggregated
results.
> +	       if build_number < aggregated_build_number:
> +		   logging.warning(("Build %d in incremental json is older than
"
> +		       "the most recent build in aggregated results: %d"),
> +		       build_number, aggregated_build_number)
> +		   return False

Maybe put in a FIXME to make this case work? It's certainly not high priority,
but we might care one day to improve on this.

> +	       # Skip merging duplicate build.
> +	       if build_number == aggregated_build_number:
> +		   logging.warning("Duplicate build %d in incremental json",
> +		       build_number)
> +		   continue

Skipping duplicated builds seems fine, but we'd need to skip the corresponding
value in _merge_tests as well. Otherwise, the results get out of sync with the
build numbers. I'd be OK with adding a FIXME for now and just returning False
here as well. Although, of course, feel free to fix it. Please add a test for
this case either way.

> +	   # Make sure results json version is updated.
> +	   aggregated_json[JSON_RESULTS_VERSION_KEY] = JSON_RESULTS_VERSION

This comment doesn't add much to the code.

> +
> +	   return file

Need newline at end of file.

> +++ WebKitTools/TestResultServer/model/jsonresults_unittest.py       
(revision 0)
> +if __name__ == '__main__':
> +    unittest.main()

Newline at end of file. :)