Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider inlining and abridging the typeCoercion.js paradigm #1308

Open
thejoshwolfe opened this issue Oct 20, 2017 · 5 comments
Open

Consider inlining and abridging the typeCoercion.js paradigm #1308

thejoshwolfe opened this issue Oct 20, 2017 · 5 comments

Comments

@thejoshwolfe
Copy link
Contributor

Currently, #1282 is going through some review feedback where the use of the harness/typeCoercion.js is causing confusion. As the creator of typeCoercion.js, I have a proposal to make its use less confusing:

  1. Remove harness/typeCoercion.js.
  2. Add a python script to tools/ that generates individual assertion cases to fill in the coverage that we were getting through typeCoercion.js.
  3. Generate updated versions of the existing tests that use typeCoercion.js.

For background, typeCoercion.js has caused a non-trivial amount of trouble for test262 ( #1241 (comment) , #1252 ).

I'm not sure the current test generation framework would be sufficient for this. Perhaps it would; I can look into it.

Currently, we're getting an awful lot of coverage from typeCoercion.js (see #1282 (comment) ), which is probably too much to list in individual assertions. We'll want to make sure we're generating useful assertions, and we don't want to waste effort testing too many things with a near 0% chance of helping anyone.

This issue thread is to discuss this possibility. Is this desirable?

@rwaldron
Copy link
Contributor

  1. Remove harness/typeCoercion.js.
  2. Add a python script to tools/ that generates individual assertion cases to fill in the coverage that we were getting through typeCoercion.js.
  3. Generate updated versions of the existing tests that use typeCoercion.js.

I give "yes" to all three

@thejoshwolfe
Copy link
Contributor Author

Here's what I'm generating so far: thejoshwolfe@1adfab8

I emitted code that does everything the current framework is doing behind the scenes. This definitely needs to be cut down before this is ready to go. For example, the ToBigInt tests have over 600 lines of code, many of which are trivially duplicated.

I'd also like to separate tests into multiple files categorized by things such as feature usage.

And I'd like to generate meaningful messages in the assert calls.

@thejoshwolfe
Copy link
Contributor Author

Another status update: I think the output is good. The generator python code could use some polish, but I'd like feedback on the JavaScript code I'm outputting:

thejoshwolfe@994e716

Things to note:

  • Tests with the BigInt feature are separated into their own file (unless the API itself requires BigInt).
  • Thorough tests of ToPrimitive edge cases are done once for each API, and in their own file ending with -toprimitive.js.
  • The -wrapped-values.js tests try wrapping values of various types (number, string, null, etc.) in simple wrappers: Object(x), @@toPrimitive, valueOf, and toString. This doesn't do the thorough testing that -toprimitive.js does.
  • Assert messages are generated with useful information, like ToIndex: ({}).toString() => "[object Object]" => NaN => 0.
  • Which throws cases go into -errors.js and which go into other files isn't very well thought out. There may be room for improvement there.
  • The generator code has the ability to do ToNumber coercion, but there's currently no API tests that use that facility. That code maybe should be removed; not sure.

@littledan
Copy link
Member

@thejoshwolfe Seems generally good to me. A couple nits:

  • If you say how the file was procedurally generated (even vaguely) it could be helpful for someone who wants to rebuild it.
  • You may want to do a line wrap at 100 chars.

@thejoshwolfe
Copy link
Contributor Author

If you say how the file was procedurally generated (even vaguely) it could be helpful for someone who wants to rebuild it.

Yes. That's part of my plan. I want the generator script to scan for and run .py "src" scripts like how the existing generator scans for .case and .template files. Then it will definitely make sense to put the source path in the generated output.

You may want to do a line wrap at 100 chars.

I was noticing the very long lines. I'll look into some simple heuristic, like any case that uses a wrapper object gets wrapped. This isn't the same as wrapping at 100 chars; there may still be lines longer than 100 chars with this strategy, but it's something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants