Bug 209964 – Re-enable previously skipped mips tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Paulo Matos]

Re-enable previously skipped mips tests

[Paulo Matos]

Created attachment 395373 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 395373 [details]
Patch

cq- since `jsc` tests are failing.  Can't land with tests failing.

<https://ews-build.webkit.org/#/builders/1/builds/10294>

[Paulo Matos]

Created attachment 396400 [details]
Patch

[Paulo Matos]

Created attachment 396518 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 396518 [details]
Patch

This seems fine now, but I'd prefer to let a JSC expert review in case there's something subtle I'm missing.

[Mark Lam]

Comment on attachment 396518 [details]
Patch

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

> JSTests/stress/generator-cell-with-type.js:2
> +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)

Is there a reason the following wouldn't work?
//@ requireOptions("-e 'let hardness=1e5'") if ["mips"].include?($architecture)

I expect requireOptions(x, y, z) to read as the test needing options x, y, and z.  However, the way you have it above reads like requiring option "-e" and option "let hardness=1e5", which I think is weird.  Would be nice if we can avoid this.

> JSTests/stress/generator-cell-with-type.js:4
> +hardness = typeof(hardness) === 'undefined' ? 1e6 : hardness;

Let's just call this "iterations" instead "hardness".

[Paulo Matos]

(In reply to Mark Lam from comment #6)
> Comment on attachment 396518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396518&action=review
> 
> > JSTests/stress/generator-cell-with-type.js:2
> > +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)
> 
> Is there a reason the following wouldn't work?
> //@ requireOptions("-e 'let hardness=1e5'") if
> ["mips"].include?($architecture)
> 

That was the first thing I tried but it doesn't work:
Could not open file: -e 'let hardness=1e5'

Unfortunately when the script calls the command it passes the whole thing as a single argument and jsc seems to think it's a file. I looked into this earlier on when committed this r259114.

> I expect requireOptions(x, y, z) to read as the test needing options x, y,
> and z.  However, the way you have it above reads like requiring option "-e"
> and option "let hardness=1e5", which I think is weird.  Would be nice if we
> can avoid this.
> 
> > JSTests/stress/generator-cell-with-type.js:4
> > +hardness = typeof(hardness) === 'undefined' ? 1e6 : hardness;
> 
> Let's just call this "iterations" instead "hardness".


Sure - I will push a new patch straight away.

[Paulo Matos]

Created attachment 396628 [details]
Patch

[Paulo Matos]

Created attachment 396629 [details]
Patch

[Mark Lam]

Comment on attachment 396629 [details]
Patch

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

I noticed that you removed the change in jsc-run-stress-tests that added quoting around options with white spaces.  Did that turn out to not be needed after all?

> JSTests/stress/generator-cell-with-type.js:2
> +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)

Why keep the hardness name at all?  What does "hardness" mean?  What's happening here is that you're trying to override the iterations.  Why not just call it that?

> JSTests/stress/generator-cell-with-type.js:4
> +iterations = typeof(hardness) === 'undefined' ? 1e6 : hardness;

Ditto.

[Paulo Matos]

(In reply to Mark Lam from comment #10)
> Comment on attachment 396629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396629&action=review
> 
> I noticed that you removed the change in jsc-run-stress-tests that added
> quoting around options with white spaces.  Did that turn out to not be
> needed after all?
> 

Correct. That was for bytecode-cache tests, which unfortunately cannot run when we pass options with -e, so I removed it.

> > JSTests/stress/generator-cell-with-type.js:2
> > +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)
> 
> Why keep the hardness name at all?  What does "hardness" mean?  What's
> happening here is that you're trying to override the iterations.  Why not
> just call it that?
>

Ah - from your previous comment, I thought you wanted me to use hardness and iterations instead of just hardness. I understand now you just want to use iterations. In the mind of a non-native english speaker it means how hard something is supported to be... in any case, happy to use iterations.
 
> > JSTests/stress/generator-cell-with-type.js:4
> > +iterations = typeof(hardness) === 'undefined' ? 1e6 : hardness;
> 
> Ditto.

Sure.

[Paulo Matos]

Created attachment 396742 [details]
Patch

[Paulo Matos]

Created attachment 396759 [details]
Patch

[Paulo Matos]

Created attachment 396760 [details]
Patch

[Mark Lam]

Comment on attachment 396760 [details]
Patch

r=me

[EWS]

Committed r260421: <https://trac.webkit.org/changeset/260421>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396760 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62098580>