String.prototype.{ trimStart, trimEnd, trimLeft, trimRight } tests #1246
Conversation
rwaldron
changed the title
| Unless otherwise specified, the length property of a built-in Function | ||
| object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| [[Configurable]]: true }. | ||
| includes: [propertyHelper.js] |
There was a problem hiding this comment.
features: [string-trimming] (and all files in this PR)
There was a problem hiding this comment.
I will add string-trimming to FEATURES.txt
There was a problem hiding this comment.
Please don't add it in the master branch yet. This is still a stage 2 feature. This should be added to this same branch.
| Unless otherwise specified, the length property of a built-in Function | ||
| object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| [[Configurable]]: true }. | ||
| includes: [propertyHelper.js] |
There was a problem hiding this comment.
features: [String.prototype.trimRight] (here and all trimRight test files)
There was a problem hiding this comment.
use features: [string-trimming] for consistency
| includes: [propertyHelper.js] | ||
| ---*/ | ||
|
|
||
| assert.sameValue(String.prototype.trimRight.name, "valueOf"); |
| includes: [propertyHelper.js] | ||
| ---*/ | ||
|
|
||
| assert.sameValue(String.prototype.trimStart.name, "valueOf"); |
There was a problem hiding this comment.
This may appear elsewhere, please update as needed
|
|
||
| verifyNotEnumerable(String.prototype.trimLeft, "name"); | ||
| verifyNotWritable(String.prototype.trimLeft, "name"); | ||
| verifyConfigurable(String.prototype.trimLeft, "name"); |
There was a problem hiding this comment.
You can also use:
verifyProperty(String.prototype.trimLeft, "name", {
value: "trimLeft",
enumerable: false,
writable: false,
configurable: true,
});There was a problem hiding this comment.
let's use verifyProperty instead of the other functions in the other files as well, we are trying to get rid of the legacy stuff over Test262.
| includes: [propertyHelper.js] | ||
| ---*/ | ||
|
|
||
| assert.sameValue(String.prototype.trimLeft.name, "valueOf"); |
|
|
||
| verifyNotEnumerable(String.prototype, "trimLeft"); | ||
| verifyWritable(String.prototype, "trimLeft"); | ||
| verifyConfigurable(String.prototype, "trimLeft); |
|
|
||
| verifyNotEnumerable(String.prototype, "trimRight"); | ||
| verifyWritable(String.prototype, "trimRight"); | ||
| verifyConfigurable(String.prototype, "trimRight); |
| ---*/ | ||
|
|
||
| var trimEnd = String.prototype.trimEnd; | ||
| var symbol = Symbol() |
| ---*/ | ||
|
|
||
| var trimStart = String.prototype.trimStart; | ||
| var symbol = Symbol() |
| // This code is governed by the BSD license found in the LICENSE file. | ||
|
|
||
| /*--- | ||
| esid: pending |
There was a problem hiding this comment.
I think it's safe to use sec-string.prototype.trimleft for the esid. This applies to the other files too
|
|
||
| verifyNotEnumerable(String.prototype.trimLeft, "name"); | ||
| verifyNotWritable(String.prototype.trimLeft, "name"); | ||
| verifyConfigurable(String.prototype.trimLeft, "name"); |
There was a problem hiding this comment.
let's use verifyProperty instead of the other functions in the other files as well, we are trying to get rid of the legacy stuff over Test262.
|
|
||
| /*--- | ||
| esid: pending | ||
| description: Behavoir when "this" value is a boolean. |
There was a problem hiding this comment.
typo on Behavior.
missing info and feature tags.
There was a problem hiding this comment.
I thought "info" wasn't mandatory? Especially because this is a success case, I wasn't sure what text from ecmascript to include..
There was a problem hiding this comment.
while it's not mandatory, we are trying to enforce its use with the matching spec text for the unit being tested. It helps a ton while reading the test with a cross reference.
There was a problem hiding this comment.
Runtime Semantics: TrimString ( string, where )
1. Let str be ? RequireObjectCoercible(string).
2. Let S be ? ToString(str).
...
ToString ( argument )
- If argument is true, return "true".
- If argument is false, return "false".
There was a problem hiding this comment.
Later Friday day, after making this comment, I came to the same conclusion as you! Commit here:
4587c81
But I didn't include quite as much information, "TrimString" instead of "Runtime Semantics: TrimString (string, where)". I think I should add the extra information for extra clarity.
| assert.sameValue( | ||
| trimStart.call(true), | ||
| 'true', | ||
| 'String.prototype.trimStart.call(true)' |
There was a problem hiding this comment.
Thanks so much for adding the assertion messages! It helps identifying them.
rwaldron
changed the title
spectranaut
force-pushed
the
trims
branch
3 times, most recently
from
eea3252 to
c13978c
Compare
|
The current set is looking good to me. We can't merge anything yet as this is still on stage 2. |
| }, | ||
| toString: function() { | ||
| throw new Test262Error( | ||
| 'this.toString called before this[Symbol.toPrimitive]' |
| ... | ||
|
|
||
| ToString ( argument ) | ||
| If arguement is Object: |
| /*--- | ||
| esid: sec-String.prototype.trimStart | ||
| description: > | ||
| ToString perfers Symbol.toPrimitive to toString to valueOf |
There was a problem hiding this comment.
This is confusing:
to toString to valueOf
| @@ -0,0 +1,95 @@ | |||
| // Copyright (C) 2017 the Valerie Young. All rights reserved. | |||
There was a problem hiding this comment.
I know you're the Valerie Young, but maybe just your name (no the) is fine here. :)
There was a problem hiding this comment.
the Valerie Young
Excellent.
| trimStart.call(thisVal); | ||
| assert.sameValue(called, 1, '[Symbol.toPrimitive] expected to have been called'); | ||
|
|
||
| var called = 0; |
There was a problem hiding this comment.
this feels so much like deserving a separate file. You even redeclare called and thisVal, looks like ready to find a new home in a new file.
| }; | ||
|
|
||
| // Test that valueOf is called when neither [Symbol.toPrimitive] nor toString | ||
| // are defined. |
| toString: undefined, | ||
| get valueOf() { | ||
| called += 1; | ||
| return function() { return '' }; |
There was a problem hiding this comment.
here you check it gets valueOf once, but not that it calls it.
Also, returning some other string might help to verify the result, e.g.: return ' 42 ';
leobalter
changed the title
|
In a first round of review for all the commits, there isn't anything I'm missing or that needs a fix. I'll check the changes locally but it seems great so far. |
| ---*/ | ||
|
|
||
| verifyProperty(String.prototype.trimLeft, "name", { | ||
| value: "trimLeft", |
There was a problem hiding this comment.
Is this correct? According to the proposal’s README the name should be trimStart, although I agree the currently proposed spec text doesn’t make that happen. +@sebmarkbage @evilpie @ljharb
There was a problem hiding this comment.
It should be trimStart. Nice catch.
There was a problem hiding this comment.
The tests in test/built-ins/String/prototype/* are trimEnd and trimStart, these are annex B
There was a problem hiding this comment.
That's the confusing part of how this is speced. The property name is what is defined in annex B, but even in annex B, this is the same function instance as trimStart so it has the same functionInstance.name field by definition.
Note that it otherwise gets impossible to pass both these name tests and the same value equality test:
https://github.com/tc39/test262/pull/1246/files#diff-c045566571ceee76729db55485a41779
You can't have the same value with two different names.
There was a problem hiding this comment.
It took me a second to see what I misunderstood. I'll fix this locally before pushing to master
There was a problem hiding this comment.
The function object that is the initial value of String.prototype.trimLeft is the same function object that is the initial value of String.prototype.trimStart.
Same as the function in Array.prototype[Symbol.iterator] is named values, trimLeft.name is "trimStart".
thanks for catching this.
| ---*/ | ||
|
|
||
| verifyProperty(String.prototype.trimRight, "name", { | ||
| value: "trimRight", |
There was a problem hiding this comment.
Yes, trimLeft and trimRight are Annex B features, but the tests should still match the specified behavior.
There was a problem hiding this comment.
Yep, I see the oversight now, thanks for your patience :)
There was a problem hiding this comment.
The names still need to be updated like @mathiasbynens pointed out.
The "name" property of String.prototype.trimLeft should be "trimStart".
The "name" property of String.prototype.trimRight should be "trimEnd".
|
Landed |
| var trimEnd = String.prototype.trimEnd; | ||
|
|
||
| // A string of all valid WhiteSpace Unicode code points | ||
| var wspc = '\u0009\u000B\u000C\u0020\u00A0\FEFF\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000'; |
| var trimStart = String.prototype.trimStart; | ||
|
|
||
| // A string of all valid WhiteSpace Unicode code points | ||
| var wspc = '\u0009\u000B\u000C\u0020\u00A0\FEFF\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000'; |
There was a problem hiding this comment.
\FEFF should be \uFEFF. Also, some whitespace code points are missing. PR incoming.
This patch fixes a typo (`\FEFF` → `\uFEFF`) and adds some missing whitespace symbols as a follow-up to tc39#1246.
This patch fixes a typo (`\FEFF` → `\uFEFF`) and adds some missing whitespace symbols as a follow-up to #1246.
No description provided.