Summary: | Re-enable previously skipped mips tests | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Paulo Matos <pmatos> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Paulo Matos <pmatos> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | cathiechen, ddkilzer, mark.lam, webkit-bug-importer, ysuzuki | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Paulo Matos
2020-04-03 07:19:41 PDT
Created attachment 395373 [details]
Patch
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> Created attachment 396400 [details]
Patch
Created attachment 396518 [details]
Patch
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.
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". (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. Created attachment 396628 [details]
Patch
Created attachment 396629 [details]
Patch
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. (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. Created attachment 396742 [details]
Patch
Created attachment 396759 [details]
Patch
Created attachment 396760 [details]
Patch
Comment on attachment 396760 [details]
Patch
r=me
Committed r260421: <https://trac.webkit.org/changeset/260421> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396760 [details]. |