Bug 38599 – Merge incremental results.json on test results app engine server

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Victor Wang]

We have an App Engine server (test-results.appsopt.com) to host test results json files. Currently, the app engine only allows client to upload aggregated json results and replace the one stored on server. Add merging functionality to the App Engine so client could upload incremental results.json.

[Victor Wang]

Created attachment 55163 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 55163 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
0:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/TestResultServer/simplejson/_speedups.c:152:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:165:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:168:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:175:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:186:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:188:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKitTools/TestResultServer/simplejson/_speedups.c:204:  speedups_methods is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:12:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:41:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:46:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:64:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:70:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:77:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:84:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:105:  whitespace before ')'  [pep8/E202] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:108:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:116:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:173:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:220:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:236:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:243:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:269:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:278:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:287:  blank line at end of file  [pep8/W391] [5]
Total errors found: 79 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Victor Wang]

Created attachment 55165 [details]
Fix styles in ChangeLog

There are style warnings in simplejson model. This is third party lib so don;t think we should touch it.

[WebKit Review Bot]

Attachment 55165 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
ank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:21:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:29:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:46:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:50:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:68:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:111:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:118:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:159:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:160:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:185:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:196:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:201:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/decoder.py:234:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:12:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:41:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:46:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:64:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:70:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:77:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:84:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:105:  whitespace before ')'  [pep8/E202] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:108:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:116:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:173:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:220:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:236:  trailing whitespace  [pep8/W291] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:243:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:269:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:278:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/TestResultServer/simplejson/__init__.py:287:  blank line at end of file  [pep8/W391] [5]
Total errors found: 77 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Victor Wang]

These warnings are from third party lib: simplejson.

(In reply to comment #4)
> Attachment 55165 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']"
> exit_code: 1
> Last 3072 characters of output:
> ank lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:21:  expected 2 blank lines,
> found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:29:  expected 2 blank lines,
> found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:46:  expected 2 blank lines,
> found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:50:  expected 2 blank lines,
> found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:68:  expected 2 blank lines,
> found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:111:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:118:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:159:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:160:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:185:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:196:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:201:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/decoder.py:234:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:12:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:41:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:46:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:64:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:70:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:77:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:84:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:105:  whitespace before ')'
>  [pep8/E202] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:108:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:116:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:173:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:220:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:236:  trailing whitespace 
> [pep8/W291] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:243:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:269:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:278:  expected 2 blank
> lines, found 1  [pep8/E302] [5]
> WebKitTools/TestResultServer/simplejson/__init__.py:287:  blank line at end of
> file  [pep8/W391] [5]
> Total errors found: 77 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 55165 [details]
Fix styles in ChangeLog

simplejson is already available autoinstalled from webkitpy.thirdparty.simplejson.

[Eric Seidel (no email)]

Comment on attachment 55165 [details]
Fix styles in ChangeLog

Oh.  I see.  This is on app-engine so we have to include our own copy.

The .c file seems useless in that case anyway.  Since AppEngine won't use it since it only uses .py files, no?

[Eric Seidel (no email)]

I expect there is some slick way by which we could configure AppEngine to upload a specific version of simplejson w/o checking one in.  But it should also be OK to check this in if MIT is allowed in WebKit (which I think it might be, but I"m not sure).

[Adam Barth]

I think AppEngine might have a JSON library already.

[Adam Barth]

from django.utils import simplejson

according to http://code.google.com/appengine/articles/rpc.html

[Victor Wang]

Created attachment 55176 [details]
Remove simplejson from AE

Remove simplejson from this AE and update related code. Thanks Adam!

[Kinuko Yasuda]

LGTM though I'm not a reviewer.
Some nits and comments:

> --- WebKitTools/ChangeLog	(revision 58840)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,29 @@
> [phone]  Victor Wang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        -. Add result.json incremental merging functionality to test results app engine.
> +        -. blobstore is not suitable for AE data merging and there is no API to
> +           programatically edit existing blob or write a new one yet, so replace blbostore

blbostore -> blobstore

> --- WebKitTools/TestResultServer/model/jsonresults.py	(revision 0)
> +++ WebKitTools/TestResultServer/model/jsonresults.py	(revision 0)
> +class JsonResults(object):
> +    @classmethod

This class only has classmethods?
I have no strong opinion here but is there a reason not to use module-level functions?

> +    @classmethod
> +    def merge(cls, builder, aggregated, incremental, sort_keys=False):
<snip>
> +        return JsonResults._generate_file_data(aggregated_json, sort_keys)

return cls._generate_file_data(aggregated_json, sort_keys)
might be better?

> --- WebKitTools/TestResultServer/model/jsonresults_unittest.py	(revision 0)
> +++ WebKitTools/TestResultServer/model/jsonresults_unittest.py	(revision 0)
> +        # Test if build in incremental results is older than the most recent
> +        # build in aggregated results.
> +        # The incremental results should be dropped and no merge happens.
> +        self._test_merge(
> +            # Aggregated results
> +            (["3", "1"], [["001.html", "[200,\"P\"]", "[200,\"0\"]"]]),
> +            # Incremental results
> +            (["2"], [["001.html", "[1, \"P\"]", "[1,\"0\"]"]]),
> +            # Expected no merge happens.
> +            None)

Can this happen in real case?  In that case do we really want to leave it with no merge?  (Just want to make sure)

[Victor Wang]

(In reply to comment #12)
> LGTM though I'm not a reviewer.
> Some nits and comments:
> 
> > --- WebKitTools/ChangeLog	(revision 58840)
> > +++ WebKitTools/ChangeLog	(working copy)
> > @@ -1,3 +1,29 @@
> > [phone]  Victor Wang  <[e-mail] (c)>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        -. Add result.json incremental merging functionality to test results app engine.
> > +        -. blobstore is not suitable for AE data merging and there is no API to
> > +           programatically edit existing blob or write a new one yet, so replace blbostore
> 
> blbostore -> blobstore
done

> 
> > --- WebKitTools/TestResultServer/model/jsonresults.py	(revision 0)
> > +++ WebKitTools/TestResultServer/model/jsonresults.py	(revision 0)
> > +class JsonResults(object):
> > +    @classmethod
> 
> This class only has classmethods?
> I have no strong opinion here but is there a reason not to use module-level
> functions?
No strong reason. I was thinking this as a util class so did it this way. I am fine either way. If you strong prefer to do it other way, I am OK to change it.

> 
> > +    @classmethod
> > +    def merge(cls, builder, aggregated, incremental, sort_keys=False):
> <snip>
> > +        return JsonResults._generate_file_data(aggregated_json, sort_keys)
> 
> return cls._generate_file_data(aggregated_json, sort_keys)
> might be better?
done

> 
> > --- WebKitTools/TestResultServer/model/jsonresults_unittest.py	(revision 0)
> > +++ WebKitTools/TestResultServer/model/jsonresults_unittest.py	(revision 0)
> > +        # Test if build in incremental results is older than the most recent
> > +        # build in aggregated results.
> > +        # The incremental results should be dropped and no merge happens.
> > +        self._test_merge(
> > +            # Aggregated results
> > +            (["3", "1"], [["001.html", "[200,\"P\"]", "[200,\"0\"]"]]),
> > +            # Incremental results
> > +            (["2"], [["001.html", "[1, \"P\"]", "[1,\"0\"]"]]),
> > +            # Expected no merge happens.
> > +            None)
> 
> Can this happen in real case?  In that case do we really want to leave it with
> no merge?  (Just want to make sure)
Not sure whether this happens in real case, but in theory, it may happen. If it happens, I am not sure a good way to merge the test results and times because it requires the values to be summed from continuous runs.
Here is one example: build 1 test result is [1, "P"], then build 3 result [1, "P"] comes first, so the merged results would be [2, "p"], however, if build 2 result is [1, "I"], then the previous build 3 merging is not right, but at that time, it is hard to tell how you want to merge it. To simplify this, for now, I skip the merging if the incremental build is older than the most recent build in aggregated results. Would like to see how it happens in real case and what's the best way to handle this.

[Victor Wang]

Created attachment 55288 [details]
Address Kinuko's comments

[Kinuko Yasuda]

(In reply to comment #13)
> > Can this happen in real case?  In that case do we really want to leave it with
> > no merge?  (Just want to make sure)
> Not sure whether this happens in real case, but in theory, it may happen. If it
> happens, I am not sure a good way to merge the test results and times because
> it requires the values to be summed from continuous runs.
> Here is one example: build 1 test result is [1, "P"], then build 3 result [1,
> "P"] comes first, so the merged results would be [2, "p"], however, if build 2
> result is [1, "I"], then the previous build 3 merging is not right, but at that
> time, it is hard to tell how you want to merge it. To simplify this, for now, I
> skip the merging if the incremental build is older than the most recent build
> in aggregated results. Would like to see how it happens in real case and what's
> the best way to handle this.

Thanks for explaining the rational, your comments and changes sound/look good to me.

[Eric Seidel (no email)]

Comment on attachment 55288 [details]
Address Kinuko's comments

WebKitTools/TestResultServer/main.py: 
 +      ('/uploadfail', testfilehandler.UploadStatus),
So this new incremental based upload replaces the old type of upload?

WebKitTools/TestResultServer/handlers/testfilehandler.py:68
 +  class GetFile(webapp.RequestHandler):
No sure what this change does...

WebKitTools/TestResultServer/handlers/testfilehandler.py:163
 +              if type(item) is list:
Strange.  I would have probably done:
if not isinstance(item, list) and not isinstance(tuple, item):
    item = [item]
files.extend(item)

Will your code have trouble with tuples?  Seems it might.

WebKitTools/TestResultServer/handlers/testfilehandler.py:169
 +          for f in files:
bad bad single-char variable names. :(

WebKitTools/TestResultServer/handlers/testfilehandler.py:172
 +                  if f.filename != "results.json":
case sensitive file systems?

WebKitTools/TestResultServer/handlers/testfilehandler.py:177
 +                  tf = JsonResults.update(builder, test_type, f.value)
What the heck is tf?

WebKitTools/TestResultServer/model/datastorefile.py:39
 +      """Datastore entry that stores one segmant of file data
segment

WebKitTools/TestResultServer/model/datastorefile.py:38
 +  class DataEntry(db.Model):
I'm confused.  So we end up splitting the uploaded files into pieces instead of storing them in blobs?

WebKitTools/TestResultServer/model/datastorefile.py:55
 +         If a file is oversize (>1000*1000 bytes), the file is splitted into
s/splitted/split/

WebKitTools/TestResultServer/model/datastorefile.py:53
 +  class DataStoreFile(db.Model):
I'm confused why we need our own model type for this?  The QueueStatusServer just uses plain old blobs.

WebKitTools/TestResultServer/model/jsonresults.py:49
 +      def _strip_prefix_suffix(cls, data):
This method is fragile with changes to JSON_RESULTS_PREFIX and JSON_RESULTS_SUFFIX.

WebKitTools/TestResultServer/model/jsonresults.py:59
 +          return data[len(JSON_RESULTS_PREFIX):
Maybe we should ASSERT that the suffix and prefix are as expected in the data?

WebKitTools/TestResultServer/model/jsonresults.py:103
 +      def _merge_json(cls, aggregated_json, incremental_json):
This method is rather large and could be split into smaller bits.

WebKitTools/TestResultServer/model/jsonresults.py:188
 +                      incremental_json[test_name][JSON_RESULTS_RESULTS],
I might have used local variables for incremental_json[test_name] and agregated_json[test_name]

WebKitTools/TestResultServer/model/jsonresults.py:215
 +                  for i in reversed(range(1, len(aggregated_item))):
I'm surprised python doesn't have a more concise way to express "reversed(range(1, len(aggregated_item)))"

This is a really big change to try and digest at once. :(

[Victor Wang]

(In reply to comment #16)
> (From update of attachment 55288 [details])
> WebKitTools/TestResultServer/main.py: 
>  +      ('/uploadfail', testfilehandler.UploadStatus),
> So this new incremental based upload replaces the old type of upload?
No. If the "incremental" parameter appears in the request, AE merges the uploaded results.json with the one in datastore. Otherwise, it replaces the old one. Basically there are two major changes in this patch: one is merging the incremental results.json and the other one is changing BlobStore to blob entry in DataStore because BlobStore does not support programmatically updating existing entries (confirmed with Google AE team). Since we don't use blobstore which requires redirect at the end of the request handling, the above handlers (testfilehandler.UploadStatus, GetUploadUrl etc) are no longer needed.

> 
> WebKitTools/TestResultServer/handlers/testfilehandler.py:68
>  +  class GetFile(webapp.RequestHandler):
> No sure what this change does...
As I mentioned above, blobstore is no longer used, so change the base request handler here.

> 
> WebKitTools/TestResultServer/handlers/testfilehandler.py:163
>  +              if type(item) is list:
> Strange.  I would have probably done:
> if not isinstance(item, list) and not isinstance(tuple, item):
>     item = [item]
> files.extend(item)
> 
> Will your code have trouble with tuples?  Seems it might.
Good suggestion. done.

> 
> WebKitTools/TestResultServer/handlers/testfilehandler.py:169
>  +          for f in files:
> bad bad single-char variable names. :(
changed

> 
> WebKitTools/TestResultServer/handlers/testfilehandler.py:172
>  +                  if f.filename != "results.json":
> case sensitive file systems?
done

> 
> WebKitTools/TestResultServer/handlers/testfilehandler.py:177
>  +                  tf = JsonResults.update(builder, test_type, f.value)
> What the heck is tf?
changed the name.

> 
> WebKitTools/TestResultServer/model/datastorefile.py:39
>  +      """Datastore entry that stores one segmant of file data
> segment
> 
> WebKitTools/TestResultServer/model/datastorefile.py:38
>  +  class DataEntry(db.Model):
> I'm confused.  So we end up splitting the uploaded files into pieces instead of storing them in blobs?
Yes, due to the size limitation of datastore blob entry, we need to split the uploaded file into pieces if the file is too big. Checked with AE team, this is the best we can do to support server side merging.

> 
> WebKitTools/TestResultServer/model/datastorefile.py:55
>  +         If a file is oversize (>1000*1000 bytes), the file is splitted into
> s/splitted/split/
done

> 
> WebKitTools/TestResultServer/model/datastorefile.py:53
>  +  class DataStoreFile(db.Model):
> I'm confused why we need our own model type for this?  The QueueStatusServer just uses plain old blobs.
QueueStatusServer does not expect the file size over 1M, but test results json could be over this limit.

> 
> WebKitTools/TestResultServer/model/jsonresults.py:49
>  +      def _strip_prefix_suffix(cls, data):
> This method is fragile with changes to JSON_RESULTS_PREFIX and JSON_RESULTS_SUFFIX.
> 
> WebKitTools/TestResultServer/model/jsonresults.py:59
>  +          return data[len(JSON_RESULTS_PREFIX):
> Maybe we should ASSERT that the suffix and prefix are as expected in the data?
Good suggestion. done.

> 
> WebKitTools/TestResultServer/model/jsonresults.py:103
>  +      def _merge_json(cls, aggregated_json, incremental_json):
> This method is rather large and could be split into smaller bits.
done

> 
> WebKitTools/TestResultServer/model/jsonresults.py:188
>  +                      incremental_json[test_name][JSON_RESULTS_RESULTS],
> I might have used local variables for incremental_json[test_name] and agregated_json[test_name]
done

> 
> WebKitTools/TestResultServer/model/jsonresults.py:215
>  +                  for i in reversed(range(1, len(aggregated_item))):
> I'm surprised python doesn't have a more concise way to express "reversed(range(1, len(aggregated_item)))"
Ya, I couldn't find a better way.

> 
> This is a really big change to try and digest at once. :(

[Victor Wang]

Created attachment 56280 [details]
Updated patch per Eric's comments.

[Adam Barth]

I tried to review your patch, but it was large and I got lost.  Will try again later.

[Ojan Vafai]

Comment on attachment 56280 [details]
Updated patch per Eric's comments.

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. :)

[Victor Wang]

Created attachment 63390 [details]
Updated patch per Ojan's comments

[Victor Wang]

(In reply to comment #20)
> (From update of attachment 56280 [details])
> 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.
This will cause PEP8 style warning:  blank line at end of file  [pep8/W391]

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

> 
> > +            # 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.
done

> 
> > +            # 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.
It is little tricky in handling the property data for each test in this case. The property data (results) is accumulated value, so need to be careful on how to drop or modify its value for the duplicated build number. Added a FIXME for now. Also added a test case.

> 
> > +        # 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.
removed

> 
> > +
> > +        return file
> 
> Need newline at end of file.
see above

> 
> > +++ WebKitTools/TestResultServer/model/jsonresults_unittest.py	(revision 0)
> > +if __name__ == '__main__':
> > +    unittest.main()
> 
> Newline at end of file. :)
see above

[Victor Wang]

Created attachment 63475 [details]
Updated Patch

Remove one debugging statement (print request) from the previous patch.

[Victor Wang]

Comment on attachment 63475 [details]
Updated Patch

Clearing flags on attachment: 63475

Committed r64687: <http://trac.webkit.org/changeset/64687>

[Victor Wang]

All reviewed patches have been landed.  Closing bug.