1149769 - Split tests/js1_8_5/extensions/reflect-parse.js into its own directory

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

Attachment: Patch

This test is almost 2000 lines long. It takes about 30 seconds to run in a debug shell. It deserves to be repotted. It has outgrown its home.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8586347 [details] [diff] [review]
Patch

Review of attachment 8586347 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/js1_8_5/reflect-parse/classes.js
@@ +138,5 @@
> +    // computed property name 'constructor'
> +    assertClass("class NAME { constructor () { } [\"constructor\"] () { } }",
> +                [simpleConstructor, emptyCPNMethod("constructor", false)]);
> +
> +    // It is an error to have a generator or accessor named constructor

You could add the complementary case to this, along lines 137-140, and have computed getter/setter names that are "constructor", right?  Add that, in a followup patch probably.

@@ +215,5 @@
> +                          classStmt(ident("Foo"), null, [simpleConstructor])]));
> +
> +
> +    // Can't make a lexical binding inside a block.
> +    assertError("if (1) class Foo { constructor() { } }", SyntaxError);

s/inside/without/ was meant?

::: js/src/tests/js1_8_5/reflect-parse/general.js
@@ +1,3 @@
> +// general tests
> +
> +// NB: These are useful but for now jit-test doesn't do I/O reliably.

And maybe put these in, uh.  Are they actual tests?  Given flapjax.txt doesn't exist in js/src, I'm inclined to say you should just delete these entirely.

@@ +7,5 @@
> +//program(Program.ANY).assert(Reflect.parse(snarf('data/prototype.js')));
> +//program(Program.ANY).assert(Reflect.parse(snarf('data/dojo.js.uncompressed.js')));
> +//program(Program.ANY).assert(Reflect.parse(snarf('data/mootools-1.2.4-core-nc.js')));
> +
> +// Bug 632024: no crashing on stack overflow

I'd move this into a stack-overflow.js test.  "general.js" doesn't tell the directory-skimmer a thing about whether the test contains what he wants or not.

::: js/src/tests/js1_8_5/reflect-parse/statements.js
@@ +77,5 @@
> +                   catchClause(ident("e"), null, blockStmt([])),
> +                   blockStmt([])));
> +
> +
> +// Bug 632028: yield outside of a function should throw

I'd move this to its own file.  (Generally I'd move any obvious one-off tests into their own files.)  Feel free to do in a followup patch/rev if desired, doesn't matter to me as long as the end product is simpler.

Comment 2

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e942133b1f9b

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out for test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2167fb2ea0a1

See:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e942133b1f9b

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f138912738ed