Bug 78989 - Move more logic from handler classes to model classes and add unit tests
Summary: Move more logic from handler classes to model classes and add unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037 79018
  Show dependency treegraph
 
Reported: 2012-02-18 21:36 PST by Ryosuke Niwa
Modified: 2012-02-20 16:25 PST (History)
4 users (show)

See Also:


Attachments
Cleanup and add more tests (30.23 KB, patch)
2012-02-18 21:41 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added one more line of refactoring (30.53 KB, patch)
2012-02-18 21:48 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per morrita's comment (30.48 KB, patch)
2012-02-20 01:17 PST, Ryosuke Niwa
morrita: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-02-18 21:36:00 PST
Move more logic from handler classes to model classes and add unit tests
Comment 1 Ryosuke Niwa 2012-02-18 21:41:38 PST
Created attachment 127727 [details]
Cleanup and add more tests
Comment 2 Ryosuke Niwa 2012-02-18 21:48:20 PST
Created attachment 127728 [details]
Added one more line of refactoring
Comment 3 Hajime Morrita 2012-02-19 16:13:50 PST
Comment on attachment 127728 [details]
Added one more line of refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=127728&action=review

> Websites/webkit-perf.appspot.com/create_handler.py:85
>      def _create_branch(self, key, name):

Looks to be able to @classmethod?

> Websites/webkit-perf.appspot.com/create_handler.py:90
>      def _create_platform(self, key, name):

ditto.

> Websites/webkit-perf.appspot.com/models.py:186
> +    @staticmethod

You can use @classmethod, then TestResult.all() can be cls.get_or_insert().

> Websites/webkit-perf.appspot.com/models.py:203
> +    @staticmethod

Ditto for @classmethod.

> Websites/webkit-perf.appspot.com/runs_handler.py:64
>          test = model_from_numeric_id(test_id, Test)

The triple (branch, platorm,test) could form a class. It can be done in separate change though.

> Websites/webkit-perf.appspot.com/runs_handler.py:74
> +        for build, result in TestResult.generate_runs(branch, platform, test.name):

If "runs" were a class, We can write Runs.to_json() and test it.
Comment 4 Ryosuke Niwa 2012-02-20 01:15:12 PST
Comment on attachment 127728 [details]
Added one more line of refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=127728&action=review

>> Websites/webkit-perf.appspot.com/create_handler.py:85
>>      def _create_branch(self, key, name):
> 
> Looks to be able to @classmethod?

They could be. I don't feel the need since these functions are called at exactly one place.
Also, I'm planning to rewrite all of these admin. handlers since they're way too primitive and limited.
(They are inherited from my first prototype I write in less than a day...)

>> Websites/webkit-perf.appspot.com/create_handler.py:90
>>      def _create_platform(self, key, name):
> 
> ditto.

Ditto.

>> Websites/webkit-perf.appspot.com/models.py:186
>> +    @staticmethod
> 
> You can use @classmethod, then TestResult.all() can be cls.get_or_insert().

Done.

>> Websites/webkit-perf.appspot.com/models.py:203
>> +    @staticmethod
> 
> Ditto for @classmethod.

I didn't make this change since this method uses both Build and TestResult, and I think it's much more confusing to use cls for the latter.

>> Websites/webkit-perf.appspot.com/runs_handler.py:74
>> +        for build, result in TestResult.generate_runs(branch, platform, test.name):
> 
> If "runs" were a class, We can write Runs.to_json() and test it.

Right. But that should probably be done in a separate patch (I've tried but the diff becomes unintelligible) since manifest and dashboard JSON responses should also have their own classes.
Comment 5 Ryosuke Niwa 2012-02-20 01:17:29 PST
Created attachment 127779 [details]
Fixed per morrita's comment
Comment 6 Hajime Morrita 2012-02-20 15:40:08 PST
Comment on attachment 127779 [details]
Fixed per morrita's comment

BTW, Why did you change the python27 to python? I heard that python27 is preferable option if possible because of
performance perspective since it supports multi-threaded handler.
Comment 7 Ryosuke Niwa 2012-02-20 16:07:57 PST
Comment on attachment 127779 [details]
Fixed per morrita's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=127779&action=review

> Websites/webkit-perf.appspot.com/app.yaml:3
> -runtime: python27
> +runtime: python

Oh oops, this change to be reverted.
Comment 8 Ryosuke Niwa 2012-02-20 16:25:33 PST
Committed r108270: <http://trac.webkit.org/changeset/108270>
Comment 9 Ryosuke Niwa 2012-02-20 16:25:47 PST
Thanks for the reviews!