Bug 78989 – Move more logic from handler classes to model classes and add unit tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Move more logic from handler classes to model classes and add unit tests

[Ryosuke Niwa]

Created attachment 127727 [details]
Cleanup and add more tests

[Ryosuke Niwa]

Created attachment 127728 [details]
Added one more line of refactoring

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 127779 [details]
Fixed per morrita's comment

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r108270: <http://trac.webkit.org/changeset/108270>

[Ryosuke Niwa]

Thanks for the reviews!