Add Array.prototype.includes tests #95

domenic · 2014-09-18T00:36:09Z

Passing in my local build of V8. Let's get 'em merged and then we can update V8's test262 tests and then we can rock and roll.

bterlson · 2014-12-11T01:43:31Z

Needs to be updated with name to "includes".
Also, tests should now be located under built-ins/Array/prototype/includes.

domenic · 2014-12-12T19:47:49Z

Updated and pull request title renamed

domenic · 2014-12-15T23:46:26Z

Added tests for %TypedArray%.prototype.includes. Review on them appreciated.

smikes · 2014-12-16T13:12:08Z

LGTM, minor quibbles added as inline comments

bterlson · 2014-12-16T20:21:09Z

test/built-ins/TypedArray/prototype/includes/exception-bad-this.js

+author: Domenic Denicola
+---*/
+
+var taIncludes = Uint8Array.prototype.includes;


Probably want to use your helper to test each typed array here?

Same as my response to @smikes on a similar comment, haha. We test that they are all === to each other, so in cases where we are not testing properties of a given typed array itself, there's no need to test them all.

bterlson · 2014-12-16T20:29:01Z

Great tests @domenic.

It's interesting that there aren't tests for garbage indexes on the prototype of typed arrays, but given that TA's can't have holes I'm not sure there is any need to test this... could I suppose ensure that prototype properties that are shadowed, at index -1, and at index length aren't ever considered?

Are there proxy scenarios that are interesting here worth testing directly? Proxy of a typed array should throw due to not having the proper internal state. Easy to test, might be worth doing.

domenic · 2015-08-11T21:26:43Z

I need to amend this PR a bit (there's some invalid YAML in the typed array tests apparently). But, my question is, will you accept it now that A.p.includes is stage 3? Or should I wait until later?

domenic · 2015-08-11T21:27:40Z

I guess stage 3 is the right time to accept, since passing acceptance tests are required for stage 4.

From https://github.com/tc39/Array.prototype.includes/tree/598c7ff43d13858e59aa42c4a75c8abc2ff200c2/test

Includes new testTypedArrays.js helper.

domenic · 2015-11-17T20:03:20Z

Tests updated and ready to pull including unscopables change

leobalter · 2015-11-17T20:10:02Z

It would be great to set the es6id references too.

leobalter · 2015-11-17T20:10:46Z

test/built-ins/Array/prototype/includes/from-index-default.js

+/*---
+description: >
+    Array.prototype.includes should search the whole array, as the optional second argument fromIndex defaults to 0
+author: Robert Kowalski


is this info correct? (also the same name is on the first line)

Yes; why would it not be?

just a safe check. We've had other PRs with wrong authorship info.

Yes; why would it not be?

Because the name shown is not the name of the author of this commit—that's not an unreasonable inquiry.

@bterlson Do we have confirmation that the contributing author has signed http://tc39.github.io/test262-cla/ ?

Update: I reviewed the CLA responses and there is no "Robert Kowalski". Please have the author sign http://tc39.github.io/test262-cla/

@robertkowalski

submitted! :)

@robertkowalski confirmed—thank you very much!

domenic · 2015-11-17T20:31:41Z

It would be great to set the es6id references too.

I disagree; maintaining fragile section numbers is busywork.

leobalter · 2015-11-17T20:48:05Z

I disagree; maintaining fragile section numbers is busywork.

They are good to find a reference on the spec and check what is missing from the implementation if it breaks. The tests won't document why it should work on that way by themselves.

The contributing file also says it's necessary to use the es5id/es6id frontmatter tags: https://github.com/tc39/test262/blob/master/CONTRIBUTING.md and https://github.com/tc39/test262/blob/master/CONTRIBUTING.md#es5id

jugglinmike · 2015-11-18T15:00:47Z

Actually, as a feature proposal for the next version of ECMAScript,
Array.prototype.includes has not been assigned an identifier. I think this
trivially resolves the issue of whether or not to include an es6id in these
tests, but it forces a more fundamental question I've had about the order of
operations in the standardization process.

Although Array.prototype.includes has reached stage 4 (nice work to @domenic
and @rwaldron on that), until the publication of ES2016, it is still
non-standard. Strictly speaking, this means that ECMA has not formally
committed to the feature. From that perspective, including tests for it in the
official language test suite is somewhat contradictory.

I'm not arguing consistency for consistency's sake, though. I'm thinking of
implementors who are targeting conformance. If these tests land today,
implementors will begin failing even if they are 100% in-line with the most
recent standard.

One practical solution would be (you guessed it) additional metadata--some way
to denote that these tests describe almost-but-not-quite finalized language
features. This is effectively equivalent to es5id/es6id in that it allows
implementors to filter tests according to their expectations for their
runtimes.

@goyakin @domenic @bterlson I'm interested to hear your thoughts about this.

domenic · 2015-11-18T16:24:53Z

The agreement was stage 3 and up proposals go in test262.

jugglinmike · 2015-11-18T16:42:35Z

Thanks Domenic! From September 24th:

_ general agreement that test262 tests should be stage 3 _

I still think there's value in a dedicated tag, though, especially considering the "run in a browser" use case that was discussed in that session.

caitp · 2015-11-18T16:50:32Z

it's okay for test262 tests to fail when run in a browser (they always have, that's not anything new). I don't think there is an implementation on the market with a perfect test262 score

jugglinmike · 2015-11-18T17:00:06Z

What I have in mind is the "marketing feature" that Brian mentions in those minutes. You're right that it's not really an option now, but metadata like that would be a good step in that direction.

rwaldron · 2015-11-18T17:33:17Z

@jugglinmike I agree that there needs to be a dedicated tag, even if it looks like:

es7id: pending

(Or whatever, I'm not interested in bike shedding there)

The value is that it will make it easier to identify the tests that need to be updated with their new section numbers, when the time comes to do so.

domenic · 2015-11-18T17:37:54Z

I disagree with the division of tests into version numbers. The spec text will be in master soon. And that's what matters; spec version numbers are bulls hit.

But anyway, feel free to modify these in any way you want while merging. I'm done here unless there are substantive technical comments.

rwaldron · 2015-11-18T20:29:59Z

spec version numbers are bulls hit.

This has nothing to do with spec version numbers. There needs to be a way to correlate any given test file with the actual thing that's being tested within. The number part of "esNid" was necessary to indicate where to look in which version of the spec to understand what was being tested and why. This is critical for maintaining this large of a test suite. The request is not unreasonable.

But anyway, feel free to modify these in any way you want while merging. I'm done here unless there are substantive technical comments.

This response is really surprising and lacks any sense of respect for the committee, community, maintainers and consumers of this test suite. Surely that's not your intention? I would appreciate it if you were more willing to cooperate, I think we agree that this endeavor will be more productive when everyone is cooperating to their fullest capability. Thanks in advance.

leobalter · 2015-11-18T20:39:37Z

@bterlson, I have a suggestion. This can be merged into a pending branch instead of master. As soon as we're able to have references for the tests, we can update and merge it to master through new commits / PRs.

rwaldron · 2015-11-18T20:45:19Z

Thanks @domenic

~~These tests can't be accepted in their current form because they are missing requisite frontmatter, as described in https://github.com/tc39/test262/blob/master/CONTRIBUTING.md.~~

~~Per Frontmatter:~~

~~> There must be exactly one Frontmatter per test.~~

~~And of the supported frontmatter tags, description is required~~

*Update: on my first scan, I didn't notice that the file missing the front matter was actually a harness file, so this is not an issue. *

I'm currently reviewing the contributor CLA responses, will post back as necessary.

rwaldron · 2015-11-18T20:46:20Z

harness/testTypedArrays.js

@@ -0,0 +1,21 @@
+// Copyright (C) 2014 Domenic Denicola. All rights reserved.
+// This code is governed by the BSD license found in the LICENSE file.
+


Missing required frontmatter description tag.

This is not a test.

Indeed, sorry for the confusion there, I was scanning through all of the files quickly. I'll update my main comment accordingly.

rwaldron · 2015-11-18T21:01:10Z

@robertkowalski thanks for the contribution to @domenic's Array.prototype.incudes test suite! If you would kindly sign the Ecma International, TC39 ECMAScript Conformance Test Suites Software Contribution Form, your tests will be 👍 for inclusion!

rwaldron · 2015-11-18T21:25:59Z

test/built-ins/Array/prototype/includes/function-length.js

+author: Domenic Denicola
+---*/
+
+assert.sameValue(Array.prototype.includes.length, 1, 'Expected Array.prototype.includes.length to be 1');


Missing property verifications, eg. https://github.com/tc39/test262/blob/master/test/built-ins/Array/prototype/find/length.js

bterlson · 2015-11-18T21:50:19Z

@domenic I don't appreciate your second guessing of test262 experts. esNid is important. Experts in this test suite are telling you so. The Stage 4 requirement for test262 test implies that those tests must be appropriate test262 tests - you don't get to dump whatever you want and force someone else to clean up your mess. Had I known of this debate yesterday I'd have made clear to you that your stage 4 approval is contingent upon writing tests the test262 project feels are appropriate.

To me, es7id: pending is appropriate. I don't want a branch because I want it to be easy for implementers to run these tests along with the rest.

rwaldron · 2015-11-18T21:50:36Z

test/built-ins/TypedArray/prototype/includes/function-name.js

+---*/
+
+assert.sameValue(Uint8Array.prototype.includes.name, 'includes',
+    'Expected %TypedArray%.prototype.includes.name to be \'includes\'');


Missing property verifications, eg. https://github.com/tc39/test262/blob/master/test/built-ins/Array/prototype/find/name.js

bterlson · 2015-11-18T22:03:33Z

To clarify, I am not saying your tests are a mess (they are overall very good!), just pointing out that it's important for champions to write tests acceptable to the test262 project contributors. I hope this makes sense.

jugglinmike · 2016-06-01T19:39:20Z

Superseded by gh-643.

bterlson added the coverage label Oct 24, 2014

bterlson added the future label Dec 10, 2014

domenic force-pushed the a-p-contains branch from d87401e to e880726 Compare December 12, 2014 19:47

domenic changed the title ~~Add Array.prototype.contains tests~~ Add Array.prototype.includes tests Dec 12, 2014

bterlson reviewed Dec 16, 2014
View reviewed changes

domenic mentioned this pull request Jan 4, 2015

Put tests in separate module tc39/proposal-Array.prototype.includes#30

Closed

domenic force-pushed the a-p-contains branch from 87e722b to 9467eaa Compare November 17, 2015 18:51

domenic added 2 commits November 17, 2015 12:01

Add Array.prototype.includes tests

ae5e445

From https://github.com/tc39/Array.prototype.includes/tree/598c7ff43d13858e59aa42c4a75c8abc2ff200c2/test

Add %TypedArray%.prototype.includes tests

6b058db

Includes new testTypedArrays.js helper.

domenic force-pushed the a-p-contains branch from 9467eaa to 6b058db Compare November 17, 2015 20:02

leobalter reviewed Nov 17, 2015
View reviewed changes

rwaldron reviewed Nov 18, 2015
View reviewed changes

ljharb added a commit to ljharb/test262 that referenced this pull request Nov 23, 2015

Add es7id: pending per tc39#95 (comment)

0596e2d

ljharb added a commit to ljharb/test262 that referenced this pull request Dec 13, 2015

Add es7id: pending per tc39#95 (comment)

a44b0f3

ljharb added a commit to ljharb/test262 that referenced this pull request Dec 18, 2015

Add es7id: pending per tc39#95 (comment)

e04131e

ljharb added a commit to ljharb/test262 that referenced this pull request Dec 22, 2015

Add es7id: pending per tc39#95 (comment)

7a5385e

leobalter added awaiting author and removed future labels Apr 18, 2016

jugglinmike mentioned this pull request May 26, 2016

Add tests for Array.prototype.includes #643

Closed

jugglinmike closed this Jun 1, 2016

		@@ -0,0 +1,21 @@
		// Copyright (C) 2014 Domenic Denicola. All rights reserved.
		// This code is governed by the BSD license found in the LICENSE file.

Add Array.prototype.includes tests #95

Add Array.prototype.includes tests #95

Conversation

domenic commented Sep 18, 2014

bterlson commented Dec 11, 2014

domenic commented Dec 12, 2014

domenic commented Dec 15, 2014

smikes commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bterlson commented Dec 16, 2014

domenic commented Aug 11, 2015

domenic commented Aug 11, 2015

domenic commented Nov 17, 2015

leobalter commented Nov 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Nov 17, 2015

leobalter commented Nov 17, 2015

jugglinmike commented Nov 18, 2015

domenic commented Nov 18, 2015

jugglinmike commented Nov 18, 2015

caitp commented Nov 18, 2015

jugglinmike commented Nov 18, 2015

rwaldron commented Nov 18, 2015

domenic commented Nov 18, 2015

rwaldron commented Nov 18, 2015

leobalter commented Nov 18, 2015

rwaldron commented Nov 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaldron commented Nov 18, 2015

Choose a reason for hiding this comment

bterlson commented Nov 18, 2015

Choose a reason for hiding this comment

bterlson commented Nov 18, 2015

jugglinmike commented Jun 1, 2016