Bug 37731 – Integrate v8 testing utility with webkit tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Yaar Schnitman]

Integrate v8 testing utility with webkit tests

[Yaar Schnitman]

Created attachment 53571 [details]
Patch

[Yaar Schnitman]

Comment on attachment 53571 [details]
Patch

Please cq+ if LGTM

[Yaar Schnitman]

Eric, this refactor of the original test utility can be run to see if output is different from the reference files and optionally, to update the reference files.

I'll leave the hookup to EWS, commit queue, etc to you.

Cheers,
Yaar

[Adam Barth]

Comment on attachment 53571 [details]
Patch

+ os.system(' '.join(cmd))

os.system is a dangerous API.  Please consider using popen or webkitpy.common.system.executive.

+ webkitRoot = os.path.join(os.path.dirname(__file__), '..', '..')

This seems very fragile.  Please consider using checkout_root in webkitpy.common.checkout.scm.

+ workDir = tempfile.mkdtemp()

I don't understand why we need a working temp directory.  Can't we just generate the new output into a memory buffer?

[Yaar Schnitman]

Created attachment 53687 [details]
Patch

[Yaar Schnitman]

Comment on attachment 53687 [details]
Patch

Ready for review.

[Yaar Schnitman]

(In reply to comment #4)
> (From update of attachment 53571 [details])
> + os.system(' '.join(cmd))
> 
> os.system is a dangerous API.  Please consider using popen or
> webkitpy.common.system.executive.
Fixed to use subprocess.call

> + webkitRoot = os.path.join(os.path.dirname(__file__), '..', '..')
> 
> This seems very fragile.  Please consider using checkout_root in
> webkitpy.common.checkout.scm.
Fixed.
> 
> + workDir = tempfile.mkdtemp()
> 
> I don't understand why we need a working temp directory.  Can't we just
> generate the new output into a memory buffer?
There are 3 reasons:
1) Its hard to run 'diff' on a memory buffer.
2) Its hard to predict pipe the output of the Perl generator into a memory buffer, as there is no agreed interface for how many files and what files it outputs. While I can hard code something for V8, I want this to generalize to other generators.
3) Its shorter, simpler code.

[Yaar Schnitman]

(Please cq+ if ok)

[Adam Barth]

Comment on attachment 53687 [details]
Patch

Thanks for the patch!  Glad to have this testing harness.  :)

WebKitTools/Scripts/run-bindings-tests:105
 +      overwrite = "--overwrite" in argv
We should probably use option parser here instead of doing this manually.

WebKitTools/Scripts/run-bindings-tests:82
 +              workDir = tempfile.mkdtemp()
I still think we should do this in memory, but ok.

[WebKit Commit Bot]

Comment on attachment 53687 [details]
Patch

Clearing flags on attachment: 53687

Committed r58143: <http://trac.webkit.org/changeset/58143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.