[Webkit-unassigned] [Bug 144038] Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144038

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251460|review?                     |review-
              Flags|                            |

--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 251460
  --> https://bugs.webkit.org/attachment.cgi?id=251460
Patch

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

This patch needs a change log entry.  Run Tools/Scripts/prepare-ChangeLogs --bug=144038 and fill up the description.
r- because of this.

Can we also rename to something more redescriptive like BenchmarkRunner?

Also, I think we should put this code somewhere under Tools/Scripts/webkitpy.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:2
> +# coding : utf-8

I don't think we want add these coding encoding since we don't do that elsewhere as Chris has already pointed out.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:32
> +    parser.add_argument(
> +        '--platform',
> +        dest='platform',
> +        default='osx',
> +        choices=[
> +            'osx',
> +            'ios',
> +            'windows'],
> +        required=True)

Why don't we just put this in single line?
All these line breaks are rather making the code harder to read.
See other code in webkitpy.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:2
> +# coding : utf-8

Ditto about coding.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:7
> +class BaseBenchmarkHandle(object):

Why do we need this class at all?
It seems like we just need GenericBenchmarkHandle.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:7
> +from ..GenericHandleFactory import GenericHandleFactory

Why don't we add a wrapper script in Tools/Scripts/, e.g. run-benchmark-in-browser
so that we don't have to prefix these imports with "..".

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:17
> +    handles = json.load(
> +        open(
> +            os.path.join(
> +                os.path.dirname(__file__),
> +                handleFileName),
> +            'r'))

Again, we don't normally line break between each parenthesis like this.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandles.json:8
> +  "GenericBenchmarkHandle": {
> +    "moduleName": "GenericBenchmarkHandle", 
> +    "filePath": "BenchmarkHandle.GenericBenchmarkHandle"
> +  }, 
> +  "JetStreamBenchmarkHandle": {
> +    "moduleName": "JetStreamBenchmarkHandle", 
> +    "filePath": "BenchmarkHandle.JetStreamBenchmarkHandle"

Please do use 4-space indentation everywhere per our style guileline.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:18
> +class GenericBenchmarkHandle(BaseBenchmarkHandle):

I don't think we need to suffix all these classes with "Handle".
Just like "Manager", "Controller", etc… these suffixes don't really convey extra information.
Here, GenericBenchmark tells us just as much as GenericBenchmarkHandle.
Also, handle usually refers to a pointer like object such as a file handle so I don't think we want to use it here.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:19
> +    def prepareBenchmark(self, benchmarkPath, patch):

Why don't we call this just "prepare" since the class name already says GenericBenchmarkHandle.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:23
> +    def copyBenchmark(self, benchmarkPath):

copyBenchmark sounds very generic.  Why don't we say that we're copying to a temporary location.
e.g. _copyBenchmarkToTempDir?
Also, we normally prefix private/protected method names with underscore.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:24
> +        self.webroot = tempfile.mkdtemp()

we should be consistent in naming conventions.
either webRoot or web_root (the latter is PEP 8 style we use elsewhere for local variables and methods).

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:29
> +        self.dest = os.path.join(
> +            self.webroot,
> +            os.path.split(
> +                benchmarkPath)[1])

Seriously, all these line breaks are driving me nuts.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:35
> +    def applyPatch(self, patch):

_applyPatch.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:37
> +            oldCwd = os.getcwd()

Please use a descriptive name such as oldWorkingDirectory. "cwd" stands for current working directory
so oldCwd doesn't make much semantic sense.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:39
> +            retNum = subprocess.call(

I'd call this exitCode instead.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:44
> +                logger.info(
> +                    'Cannot apply patch, will skip current benchmarkPath')

It seems like this is an error, not an informative log, to me.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:50
> +    def clearBenchmark(self):
> +        logger.info('Cleanning Benchmark')

Why don't we call this clean?

> Tools/Scripts/PerfAutoRun/Utilities/ResultWrapper/ResultWrapperFactory.py:29
> +if __name__ == '__main__':
> +    merger = ResultWrapperFactory.create(['MergeResultWrapper'])
> +    d1 = {

Are these supposed to tests?
We usually put them in a separate tests directory.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150423/b1df4d0f/attachment.html>