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

Add tests for _TypedArray_ constructors #485

Merged
merged 7 commits into from
Feb 12, 2016

Conversation

leobalter
Copy link
Member

No description provided.

object has an [[ArrayBufferData]] internal slot.

...
9. If offset modulo elementSize ≠ 0, throw a RangeError exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually step 10 (the danger of tracking a moving target)

@jugglinmike
Copy link
Contributor

Hey Leo!

I've left some specific notes as in-line comments on this pull request, but I
also had some ideas that don't belong to any specific line:

  • The file naming scheme is a little inconsistent. Some names are prefixed with
    descriptions of the constructor under test, but others begin with a
    description of the behavior. To make discovery easier for others, I think we
    should choose one pattern and stick with it. My strong preference is to use
    the constructor as a prefix (since it mimics the general pattern of the rest
    of Test262's hierarchy), but I'd be open to arguments for the latter.
    Consistency is the important thing.

  • By testing against maximum/minimum values across all typed arrays, the precision of many of these tests is degraded. For instance, following code (adapted from buffer-argument-excessive-offset-throws.js):

    var buffer = new ArrayBuffer(8);
    
    testWithTypedArrayConstructors(function(TA) {
      assert.throws(RangeError, function() {
        new TA(buffer, 16);
      });
    });

    Triggers the error by invoking Int8Array with an offset of 16, even though
    for that constructor and that buffer, an offset of 9 would prove the intended
    behavior with greater precision.

    I would recommend calculating edge cases using TA.BYTES_PER_ELEMENT in
    order to produce more fine-grained assertions. For instance:

    testWithTypedArrayConstructors(function(TA) {
      var buffer = new ArrayBuffer(TA.BYTES_PER_ELEMENT);
      assert.throws(RangeError, function() {
        new TA(buffer, 2 * TA.BYTES_PER_ELEMENT);
      });
    });

    Although calculated values aren't as immediately readable as literals, the
    intent of the test is much more clear when the bounds are derived from a
    formula.

  • Some ideas for additional tests:

    • new TA(-0) to verify that SameValueZero (and not SameValue) is being used
      internally
    • Asserting the numeric order of traversal across properties in new TA(arrayLike)
    • Tests for the CloneArrayBuffer internal operation (it's not currently
      covered, as far as I can tell)

@leobalter
Copy link
Member Author

e74fbbb and ed387f2 covers the inline feedback. I'm working on the remaining.

@leobalter leobalter force-pushed the typedarray-constructor branch from 65198d2 to 004460b Compare February 4, 2016 20:29
@leobalter
Copy link
Member Author

Tests for the CloneArrayBuffer internal operation (it's not currently covered, as far as I can tell)

They already are, at least they're easier to find after the renaming. typedarray-arg-same-ctor-*

new TA(-0) to verify that SameValueZero (and not SameValue) is being used internally

that's a good one!

Asserting the numeric order of traversal across properties in new TA(arrayLike)

It might be covered on some of the object-arg-* files.

The file naming scheme is a little inconsistent

I hope you like it now. I have to say it was way easier for me to browser the files after this change.

I would recommend calculating edge cases using TA.BYTES_PER_ELEMENT in order to produce more fine-grained assertions

Maybe I should still improve it, but I got some changes regarding this.


Thanks for the review, @jugglinmike! It's ready for another (final?) round.

@anba
Copy link
Contributor

anba commented Feb 5, 2016

@leobalter
Copy link
Member Author

@anba you're right, I was trying to find them on the TypedArrays folder. I'm removing this last commit.

@leobalter leobalter force-pushed the typedarray-constructor branch from c7ac8aa to 0a30413 Compare February 5, 2016 13:53
@anba
Copy link
Contributor

anba commented Feb 5, 2016

Please feel free to move the file into the TypedArrays folder. IIRC I've added that test before the ES2016 typed array constructor changes.

@leobalter
Copy link
Member Author

@anba done at 75952be

I changed the filename to match the other files.

goyakin pushed a commit that referenced this pull request Feb 12, 2016
Add tests for _TypedArray_ constructors
@goyakin goyakin merged commit ac7711e into tc39:master Feb 12, 2016
@goyakin
Copy link
Member

goyakin commented Feb 12, 2016

Thanks @leobalter

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

Successfully merging this pull request may close these issues.

4 participants