Bug 78989

Created attachment 127727 [details] Cleanup and add more tests

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

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

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

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

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

Thanks for the reviews!