Test that getCanonicalLocales() returns a mutable array.

[jungshik]

Will fix #745

[littledan]

Actually, it looks from the 402 spec like only the individual elements are made non-writable, while the length is left writable (though V8 may have a bug here), so I'm not sure if this test is correct.

[leobalter]

This seems like a specific case. I suggest breaking it into a new test file with a proper description.

Even if the new assertion remains in the same test file, the assertion description should be used to explain what you want to assert there.

For a specific case where the returned array is a not frozen array you might want to provide more assertions as the existing properties' enumerability, configurability, or writability, are not prevented from being changed.

For this particular test file, you might want to check the returned value is in fact a valid array instance.

[jungshik]

I revised the test. Per @leobalter leobalter, I added a new test file (returned-objects-is-frozen-but-expandable.js).

Per @littledan , it tests if each element is frozen (not writable, not configurable but enumerable). And, it checks if |length| is writable.

Can you take another look? Thanks

[caridy]

@littledan can you elaborate more on:

Actually, it looks from the 402 spec like only the individual elements are made non-writable, while the length is left writable (though V8 may have a bug here), so I'm not sure if this test is correct.

There are no many instances in which 402 produces non-writable object. The only one that I recall is https://tc39.github.io/ecma402/#sec-supportedlocales, which has been like that forever :). In the new APIs, we are trying to avoid that trap. I wonder what makes you think that getCanonicalLocales() exhibit this behavior as well.

[littledan]

@caridy that's all that I am referring to. I was at first unclear whether that would freeze the length; on a second reading, it looks like it does, and push would fail on the old style. Therefore, this test manages to distinguish between them.

LGTM

[leobalter]

I'm not sure if I expressed it the right way, but this test is asserting the opposite of what the specs say.

CreateArrayFromList returns a regular array, so some assertions here will fail, especially those on line 21 and 22.

You can also use the propertyDescriptor helpers to confirm a property is acting as in its descriptor.

Plus, it's always better if you avoid loops for the tests. I'll try to provide some examples.

[jungshik]

I'm not sure if I expressed it the right way, but this test is asserting the opposite of what
the specs say.

CreateArrayFromList returns a regular array, so some assertions here will fail, especially those
on line 21 and 22.

Thanks a lot for the clarification ! The latest revision (updated last weekend) was based on the earlier comment by @littledan. I was not certain that his comments agrees with the spec, which is why I asked @caridy for the clarification.

[littledan]

Sorry that the polarity of my comment was unclear! It is a mutable array; I was talking about what form other locales are returned in.

[leobalter]

ok, here follows an example on how this can be verified.

Considering the initial code:

var locales = ['en-US', 'fr'];
var result = Intl.getCanonicalLocales(locales);

First you want to assert the result is an Array instance.

assert.sameValue(Object.getPrototypeOf(result), Array.prototype, 'prototype is Array.prototype');
assert.sameValue(result.constructor, Array);

Now you want to confirm each item on this array is set with the proper descriptor. First, you include the propertyHelper.js in your frontmatter includes, and than, you use the helpers:

verifyEnumerable(result, 0);
verifyWritable(result, 0);
verifyConfigurable(result, 0);

Now you call the same methods for result[1].

The verifyWritable and verifyConfigurable helpers will mess with your results, so get it again calling Intl.getCanonicalLocales(locales) one more time.

And now you want to confirm length is set in the right way, and that it is writable. We can't use verifyWritable here, but that's ok, we can assert length is a true Array length relying on the way a new value is set to it.

//  length desc is [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false
verifyNotEnumerable(result, 'length');
verifyNotConfigurable(result, 'length');

assert.sameValue(result.length, 2);
result.length = 42;
assert.sameValue(result.length, 42);
assert.throws(RangeError, function() {
  result.length = "Leo";
}, "a non-numeric value can't be set to result.length");

That secures the returned value is a regular array as in CreateArrayFromList, and nothing is frozen.

Using Array.prototype methods are not necessary if you already checked for the prototype ahead.

[leobalter]

@littledan I was at first unclear whether that would freeze the length; on a second reading, it looks like it does, and push would fail on the old style.

AFAICT, there's nothing on ecma402 saying it freezes the length. The returned length is just a regular Array length property which desc is regularly [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false.

The length value should still be writable.

[jungshik]

@leobalter Thanks again for your help. I updated the CL.

However, now that virtually everything was (re)written by you. It does not seem right for me to land. Would you make a pull request with what you suggested?

[leobalter]

the content on line 21 (result = Intl.getCanonicalLocales(locales);) needs to be repeated before this.

the property helpers mess with the property, so result needs to be reset.

[leobalter]

just a heads up: this missing line will only affect the assertion on line 29 (assert.sameValue(result.length, 2);), but it's still necessary.

[leobalter]

However, now that virtually everything was (re)written by you. It does not seem right for me to land.

I wouldn't do it if it wasn't by this PR. This is just Open Source and I'm happy with it and glad to help.

I found a last small detail to fix, comments above, then it LGTM.

[jungshik]

Thanks, @leobalter. Fixed your last comment. And, I found one more issue and fixed it (I forgot to quote 'length' in verifyEnumerable, verifyConfigurable). I also removed 'include..compareArray' line from returned-object-is-an-array.js.

Can you merge it (after taking one more look just in case)? Thank you again for all your help !