RESOLVED FIXED 209964
Re-enable previously skipped mips tests
https://bugs.webkit.org/show_bug.cgi?id=209964
Summary Re-enable previously skipped mips tests
Paulo Matos
Reported 2020-04-03 07:19:41 PDT
Re-enable previously skipped mips tests
Attachments
Patch (5.26 KB, patch)
2020-04-03 07:21 PDT, Paulo Matos
no flags
Patch (6.17 KB, patch)
2020-04-14 04:42 PDT, Paulo Matos
no flags
Patch (6.11 KB, patch)
2020-04-15 03:56 PDT, Paulo Matos
no flags
Patch (6.12 KB, patch)
2020-04-16 01:11 PDT, Paulo Matos
no flags
Patch (5.26 KB, patch)
2020-04-16 01:59 PDT, Paulo Matos
no flags
Patch (5.27 KB, patch)
2020-04-16 23:27 PDT, Paulo Matos
no flags
Patch (5.26 KB, patch)
2020-04-17 05:58 PDT, Paulo Matos
no flags
Patch (5.27 KB, patch)
2020-04-17 06:02 PDT, Paulo Matos
no flags
Paulo Matos
Comment 1 2020-04-03 07:21:08 PDT
David Kilzer (:ddkilzer)
Comment 2 2020-04-03 11:39:11 PDT
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
Comment 3 2020-04-14 04:42:45 PDT
Paulo Matos
Comment 4 2020-04-15 03:56:01 PDT
David Kilzer (:ddkilzer)
Comment 5 2020-04-15 17:35:33 PDT
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 6 2020-04-15 23:38:17 PDT
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
Comment 7 2020-04-16 01:00:07 PDT
(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
Comment 8 2020-04-16 01:11:33 PDT
Paulo Matos
Comment 9 2020-04-16 01:59:38 PDT
Mark Lam
Comment 10 2020-04-16 11:02:06 PDT
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
Comment 11 2020-04-16 23:24:47 PDT
(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
Comment 12 2020-04-16 23:27:03 PDT
Paulo Matos
Comment 13 2020-04-17 05:58:35 PDT
Paulo Matos
Comment 14 2020-04-17 06:02:03 PDT
Mark Lam
Comment 15 2020-04-17 07:13:16 PDT
Comment on attachment 396760 [details] Patch r=me
EWS
Comment 16 2020-04-21 02:52:50 PDT
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
Comment 17 2020-04-21 02:53:15 PDT
Note You need to log in before you can comment on or make changes to this bug.