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

String.prototype.{ trimStart, trimEnd, trimLeft, trimRight } tests #1246

Closed
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@spectranaut
Copy link
Contributor

spectranaut commented Sep 27, 2017

No description provided.

@spectranaut spectranaut changed the title Trims WIP Trims initial commit Sep 27, 2017

@spectranaut spectranaut force-pushed the bocoup:trims branch from 815cc6e to d07318c Sep 27, 2017

@rwaldron rwaldron changed the title WIP Trims initial commit [WIP] String.prototype.{ trimLeft, trimRight } tests Sep 27, 2017

Unless otherwise specified, the length property of a built-in Function
object has the attributes { [[Writable]]: false, [[Enumerable]]: false,
[[Configurable]]: true }.
includes: [propertyHelper.js]

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

features: [string-trimming] (and all files in this PR)

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

I will add string-trimming to FEATURES.txt

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

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]

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

features: [String.prototype.trimRight] (here and all trimRight test files)

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

use features: [string-trimming] for consistency

includes: [propertyHelper.js]
---*/

assert.sameValue(String.prototype.trimRight.name, "valueOf");

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

"valueOf" => "trimRight"

includes: [propertyHelper.js]
---*/

assert.sameValue(String.prototype.trimStart.name, "valueOf");

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

"valueOf" => "trimStart"

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

This may appear elsewhere, please update as needed


verifyNotEnumerable(String.prototype.trimLeft, "name");
verifyNotWritable(String.prototype.trimLeft, "name");
verifyConfigurable(String.prototype.trimLeft, "name");

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

You can also use:

verifyProperty(String.prototype.trimLeft, "name", {
  value: "trimLeft",
  enumerable: false,
  writable: false,
  configurable: true,
});

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

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");

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

"valueOf" => "trimLeft"


verifyNotEnumerable(String.prototype, "trimLeft");
verifyWritable(String.prototype, "trimLeft");
verifyConfigurable(String.prototype, "trimLeft);

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

Missing a closing quote


verifyNotEnumerable(String.prototype, "trimRight");
verifyWritable(String.prototype, "trimRight");
verifyConfigurable(String.prototype, "trimRight);

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

Missing closing quote

---*/

var trimEnd = String.prototype.trimEnd;
var symbol = Symbol()

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

;

---*/

var trimStart = String.prototype.trimStart;
var symbol = Symbol()

This comment has been minimized.

@rwaldron

rwaldron Sep 27, 2017

Contributor

;

// This code is governed by the BSD license found in the LICENSE file.

/*---
esid: pending

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

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");

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

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.

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

typo on Behavior.

missing info and feature tags.

This comment has been minimized.

@spectranaut

spectranaut Sep 29, 2017

Contributor

I thought "info" wasn't mandatory? Especially because this is a success case, I wasn't sure what text from ecmascript to include..

This comment has been minimized.

@leobalter

leobalter Oct 2, 2017

Member

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.

This comment has been minimized.

@leobalter

leobalter Oct 2, 2017

Member
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".

This comment has been minimized.

@spectranaut

spectranaut Oct 2, 2017

Contributor

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)'

This comment has been minimized.

@leobalter

leobalter Sep 27, 2017

Member

Thanks so much for adding the assertion messages! It helps identifying them.

@rwaldron rwaldron changed the title [WIP] String.prototype.{ trimLeft, trimRight } tests [WIP] String.prototype.{ trimStart, trimEnd, trimLeft, trimRight } tests Sep 27, 2017

@spectranaut spectranaut force-pushed the bocoup:trims branch 3 times, most recently from eea3252 to c13978c Sep 29, 2017

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Oct 2, 2017

The current set is looking good to me.

We can't merge anything yet as this is still on stage 2.

@spectranaut spectranaut force-pushed the bocoup:trims branch from edc4685 to 6e179ba Oct 3, 2017

},
toString: function() {
throw new Test262Error(
'this.toString called before this[Symbol.toPrimitive]'

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

indentation

...
ToString ( argument )
If arguement is Object:

This comment has been minimized.

@leobalter
/*---
esid: sec-String.prototype.trimStart
description: >
ToString perfers Symbol.toPrimitive to toString to valueOf

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

typo: perfers

This comment has been minimized.

@rwaldron

rwaldron Oct 3, 2017

Contributor

This is confusing:

to toString to valueOf
@@ -0,0 +1,95 @@
// Copyright (C) 2017 the Valerie Young. All rights reserved.

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

I know you're the Valerie Young, but maybe just your name (no the) is fine here. :)

This comment has been minimized.

@spectranaut

spectranaut Oct 3, 2017

Contributor

hahahaha...

This comment has been minimized.

@rwaldron

rwaldron Oct 3, 2017

Contributor

the Valerie Young

Excellent.

trimStart.call(thisVal);
assert.sameValue(called, 1, '[Symbol.toPrimitive] expected to have been called');

var called = 0;

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

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.

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

extra space

toString: undefined,
get valueOf() {
called += 1;
return function() { return '' };

This comment has been minimized.

@leobalter

leobalter Oct 3, 2017

Member

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 ';

@spectranaut spectranaut force-pushed the bocoup:trims branch from 3f68703 to d852e17 Oct 4, 2017

@leobalter leobalter changed the title [WIP] String.prototype.{ trimStart, trimEnd, trimLeft, trimRight } tests String.prototype.{ trimStart, trimEnd, trimLeft, trimRight } tests Oct 6, 2017

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Oct 6, 2017

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.

Valerie R Young added some commits Oct 6, 2017

@leobalter

This comment has been minimized.

---*/

verifyProperty(String.prototype.trimLeft, "name", {
value: "trimLeft",

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 16, 2018

Member

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

This comment has been minimized.

@sebmarkbage

sebmarkbage Jan 16, 2018

It should be trimStart. Nice catch.

This comment has been minimized.

@rwaldron

rwaldron Jan 25, 2018

Contributor

The tests in test/built-ins/String/prototype/* are trimEnd and trimStart, these are annex B

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2018

Member

That is irrelevant.

This comment has been minimized.

@sebmarkbage

sebmarkbage Jan 25, 2018

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.

This comment has been minimized.

@rwaldron

rwaldron Jan 25, 2018

Contributor

It took me a second to see what I misunderstood. I'll fix this locally before pushing to master

This comment has been minimized.

@leobalter

leobalter Jan 25, 2018

Member

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",

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 16, 2018

Member

Same here.

This comment has been minimized.

@rwaldron

rwaldron Jan 25, 2018

Contributor

This is annex B ;)

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2018

Member

Yes, trimLeft and trimRight are Annex B features, but the tests should still match the specified behavior.

This comment has been minimized.

@rwaldron

rwaldron Jan 25, 2018

Contributor

Yep, I see the oversight now, thanks for your patience :)

@sebmarkbage
Copy link

sebmarkbage left a 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".

@rwaldron

This comment has been minimized.

Copy link
Contributor

rwaldron commented Jan 25, 2018

@rwaldron

This comment has been minimized.

Copy link
Contributor

rwaldron commented Jan 25, 2018

Landed

@rwaldron rwaldron closed this Jan 25, 2018

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';

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2018

Member

\FEFF should be \uFEFF.

This comment has been minimized.

@mathiasbynens
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';

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2018

Member

\FEFF should be \uFEFF. Also, some whitespace code points are missing. PR incoming.

This comment has been minimized.

@mathiasbynens

mathiasbynens added a commit to mathiasbynens/test262 that referenced this pull request Jan 25, 2018

[string-trimming] Fix whitespace tests
This patch fixes a typo (`\FEFF` → `\uFEFF`) and adds some missing whitespace symbols as a follow-up to tc39#1246.

rwaldron added a commit that referenced this pull request Jan 25, 2018

[string-trimming] Fix whitespace tests
This patch fixes a typo (`\FEFF` → `\uFEFF`) and adds some missing whitespace symbols as a follow-up to #1246.

@mathiasbynens mathiasbynens referenced this pull request Dec 5, 2018

Open

Advance to stage 4 #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment