Add new test suite for migrated blink tests. (issue 17260002)

[mache...@chromium.org]

Reviewers: Jakob,

Message:
PTAL

The .js and .txt files are all copied from
<chromium_root>/src/third_party/WebKit/LayoutTests/fast/js(/script-tests).
The
only modification is a new license header.

Is 'blink' actually a good name for the test suite? It does not describe its
origin very well. Maybe webkit or jsc is better?

Description:
Migrate blink tests that are not relevant to blink into a new V8 test suite
called 'blink'.

This initial CL contains the new test suite code and two tests for
demonstration.

Other tests will follow in a separate CL.

Please review this at https://codereview.chromium.org/17260002/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
A + test/blink/blink.status
A + test/blink/blink.status2
A test/blink/concat-while-having-a-bad-time-expected.txt
A test/blink/concat-while-having-a-bad-time.js
A test/blink/dfg-inlining-reg-alloc-expected.txt
A test/blink/dfg-inlining-reg-alloc.js
A test/blink/resources/standalone-post.js
A test/blink/resources/standalone-pre.js
A + test/blink/testcfg.py

[jkum...@chromium.org]

"webkit" would definitely be a better name for this test suite than "blink".
After all, these are tests that the Blink project inherited from WebKit and
is
now trying to get rid of.


https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status
File test/blink/blink.status (right):

https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status#newcode28
test/blink/blink.status:28: prefix blink
This line is not needed.

https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status2
File test/blink/blink.status2 (right):

https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status2#newcode28
test/blink/blink.status2:28: []
This entire file is not needed. It is auto-generated on demand from the
.status equivalent.

https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py
File test/blink/testcfg.py (right):

https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py#newcode51
test/blink/testcfg.py:51: dirs.sort()
I think you want to filter the "resources" directory here.

https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py#newcode109
test/blink/testcfg.py:109: if not line.strip(): continue
To make this test suite pass on Android and NaCl, you'll also have to
filter the actual output lines we ignore for the "messages" test suite.

https://codereview.chromium.org/17260002/

[mache...@chromium.org]

PTAL

On 2013/06/17 14:22:52, Jakob wrote:
> "webkit" would definitely be a better name for this test suite
> than "blink".
> After all, these are tests that the Blink project inherited from WebKit
> and is
> now trying to get rid of.

Done

> https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status
> File test/blink/blink.status (right):


https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status#newcode28
> test/blink/blink.status:28: prefix blink
> This line is not needed.

Done

> https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status2
> File test/blink/blink.status2 (right):


https://codereview.chromium.org/17260002/diff/1/test/blink/blink.status2#newcode28
> test/blink/blink.status2:28: []
> This entire file is not needed. It is auto-generated on demand from the
.status
> equivalent.

Done

> https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py
> File test/blink/testcfg.py (right):


https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py#newcode51
> test/blink/testcfg.py:51: dirs.sort()
> I think you want to filter the "resources" directory here.

Done

https://codereview.chromium.org/17260002/diff/1/test/blink/testcfg.py#newcode109
> test/blink/testcfg.py:109: if not line.strip(): continue
> To make this test suite pass on Android and NaCl, you'll also have to
> filter
the
> actual output lines we ignore for the "messages" test suite.

Done


https://chromiumcodereview.appspot.com/17260002/

[jkum...@chromium.org]

LGTM with comments.


https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py
File test/webkit/testcfg.py (right):

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode52
test/webkit/testcfg.py:52: if dirname == exclude:
I guess this works, but it doesn't fit with the exclusion scheme used
right above. How about:

exclude = 'resources'
if exclude in dirs:
dirs.remove(exclude)

That way we won't even visit the 'resources' directory.
Or you could modify/extend the existing filter:

for excluded in [x for x in dirs if x.startswith('.') or x ==
'resources']:
dirs.remove(excluded)

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode122
test/webkit/testcfg.py:122: if line.startswith("#") or not line.strip():
continue
Don't you need to have this line in ActIterator too to filter out the
copyright headers?

https://chromiumcodereview.appspot.com/17260002/

[mache...@chromium.org]

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode52
> test/webkit/testcfg.py:52: if dirname == exclude:
> I guess this works, but it doesn't fit with the exclusion scheme used
> right
> above. How about:

> exclude = 'resources'
> if exclude in dirs:
> dirs.remove(exclude)

> That way we won't even visit the 'resources' directory.
> Or you could modify/extend the existing filter:

> for excluded in [x for x in dirs if x.startswith('.') or x
> == 'resources']:
> dirs.remove(excluded)

With both suggested ways, we would ignore _every_ subdir
called 'resources'. If
the test suite grows, there could be a subdir for 'resource' tests somewhere
called special_tests/resources, which we would also ignore. But we only
want to
ignore the exact dir <test root>/resources and its subdirs. I agree that
this
kind of filter looks awkward - I'd rather prefer a first-class filter object
that is assigned to the tree-walker instead of these conditions. But I
could not
find a nice python way of doing that...

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode122
> test/webkit/testcfg.py:122: if line.startswith("#") or not line.strip():
> continue
> Don't you need to have this line in ActIterator too to filter out the
copyright
> headers?

No - the test cases don't print out the license header.


https://codereview.chromium.org/17260002/

[jkum...@chromium.org]

On 2013/06/18 09:47:54, machenbach wrote:
> >

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode52
> > test/webkit/testcfg.py:52: if dirname == exclude:
> > I guess this works, but it doesn't fit with the exclusion scheme used
> right
> > above. How about:
> >
> > exclude = 'resources'
> > if exclude in dirs:
> > dirs.remove(exclude)
> >
> > That way we won't even visit the 'resources' directory.
> > Or you could modify/extend the existing filter:
> >
> > for excluded in [x for x in dirs if x.startswith('.') or x
> == 'resources']:
> > dirs.remove(excluded)
> With both suggested ways, we would ignore _every_ subdir
> called 'resources'.

Yes, that was actually my intention.

> If
> the test suite grows, there could be a subdir for 'resource' tests
> somewhere
> called special_tests/resources, which we would also ignore.

I don't think that's a problem. It should be OK to give special meaning to
the
directory name 'resources', at least within the webkit test suite.

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode122
> > test/webkit/testcfg.py:122: if line.startswith("#") or not line.strip():
> > continue
> > Don't you need to have this line in ActIterator too to filter out the
> copyright
> > headers?
> No - the test cases don't print out the license header.

Right. I got that backwards.

https://codereview.chromium.org/17260002/

[mache...@chromium.org]

https://chromiumcodereview.appspot.com/17260002/diff/15001/test/webkit/testcfg.py#newcode52
> > > test/webkit/testcfg.py:52: if dirname == exclude:
> > > I guess this works, but it doesn't fit with the exclusion scheme used
right
> > > above. How about:
> > >
> > > exclude = 'resources'
> > > if exclude in dirs:
> > > dirs.remove(exclude)
> > >
> > > That way we won't even visit the 'resources' directory.
> > > Or you could modify/extend the existing filter:
> > >
> > > for excluded in [x for x in dirs if x.startswith('.') or x ==
'resources']:
> > > dirs.remove(excluded)
> > With both suggested ways, we would ignore _every_ subdir
> called 'resources'.

> Yes, that was actually my intention.

Done. It probably doesn't matter, since the directory structure here might
stay
rather flat anyway...


https://codereview.chromium.org/17260002/

[jkum...@chromium.org]

lgtm



https://codereview.chromium.org/17260002/

[mache...@chromium.org]

PTAL at patch 4.

The izip function omits elements from the longer iterator. If the SUT
produces
more or less output then expected, this is not detected as an error.

https://codereview.chromium.org/17260002/

[mache...@chromium.org]

Patch 5 includes better formatting and fixes the iterator problem also for
the
message test suite.

https://codereview.chromium.org/17260002/

[jkum...@chromium.org]

LGTM (pending approval of the copyright headers).

https://codereview.chromium.org/17260002/