Skip to content

Add tests for String#padStart and String#padEnd#564

Closed
ljharb wants to merge 7 commits into
tc39:masterfrom
ljharb:string_pad_start_end
Closed

Add tests for String#padStart and String#padEnd#564
ljharb wants to merge 7 commits into
tc39:masterfrom
ljharb:string_pad_start_end

Conversation

@ljharb

@ljharb ljharb commented Mar 29, 2016

Copy link
Copy Markdown
Member

@jugglinmike

Copy link
Copy Markdown
Contributor

Looking good, @ljharb! The tests look valid, I just have a few ideas for
extending coverage (and one request for formatting).

The ? shorthand is really convenient, but it tends to hide branches in the
algorithm. As written, these tests only cover one of the case where an internal
operation produces an abrupt completion. This could be done by asserting a
TypeError when a Symbol is specified for the this value, and the second
argument (separate tests, of course). Call me old fashioned, but I still prefer
an ordinary object whose toString/valueOf method throws a
Test262Error--expressing the intention in that way will make the tests portable
to shimmed ES5 environments.

I like the "observable operations" tests! If you want to get really fancy (and
who doesn't want to get fancy?), you could configure these to return ordinary
objects for the "preferred" operation--that would let you observe the "hint"
behavior of ToPrimitive.

The proposal says "If fillString is undefined, [...]", and in cases like
this, I like to explicitly test other falsey values to make sure that there is
no ToBoolean funny business going on.

The tests that have multi-line descriptions should use > rather than | as
a block delimiter because newlines are not significant. (This is a distinction
I personally discovered only recently, so many of my old commits are incorrect
in this regard).

@ljharb

ljharb commented Mar 30, 2016

Copy link
Copy Markdown
Member Author

@jugglinmike I'll add separate tests that throw on the ToString and ToLength branches (which the existing observable ops tests ensure are properly called). I'll also add more non-string + falsy fillString values to cover what you've indicated, and fix the multiline descriptions.

How could I observe the "hint" behavior of toPrimitive? Or do you mean, just the nested toString/valueOf call? That seems like it could recurse forever - at which point would I stop?

@jugglinmike

Copy link
Copy Markdown
Contributor

How could I observe the "hint" behavior of toPrimitive? Or do you mean, just
the nested toString/valueOf call? That seems like it could recurse forever -
at which point would I stop?

Sorry, that was a bit unclear. Here's the text I had in mind:

[...]
3. If hint is "string", then
   a. Let methodNames be « "toString", "valueOf" ».
4. Else,
   a. Let methodNames be « "valueOf", "toString" ».
5. For each name in methodNames in List order, do
[...]

By defining the toString methods to return a non-primitive value, you'll be
able to observe that valueOf is called following toString in such cases.

-var receiver = createPrimitiveObserver('receiver', 'abc');
+var receiver = createPrimitiveObserver('receiver', {}, 'abc');

-var fillString = createPrimitiveObserver('fillString', 'def');
+var fillString = createPrimitiveObserver('fillString', {}, 'def');

@ljharb

ljharb commented Mar 30, 2016

Copy link
Copy Markdown
Member Author

ah, i see. ok cool, i'll make the changes today

@ljharb ljharb force-pushed the string_pad_start_end branch from ea57b61 to d8cbdf9 Compare March 30, 2016 19:39
@ljharb

ljharb commented Mar 30, 2016

Copy link
Copy Markdown
Member Author

Updated!


assert.sameValue(log, '|' + [
'toString:receiver',
'toString:toPrimitiveABC',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe valueOf:receive happens here as the result of toString:receive is an Object and the the valueOf will return undefined.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

understood and fixed, thanks!

@ljharb ljharb force-pushed the string_pad_start_end branch from d8cbdf9 to 3a8d604 Compare March 30, 2016 20:33

var maxLength = createPrimitiveObserver('maxLength', {}, 11);

var result = String.prototype.padEnd.call(receiver, fillString, maxLength);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-var result = String.prototype.padEnd.call(receiver, fillString, maxLength);
+var result = String.prototype.padEnd.call(receiver, maxLength, fillString);

edit: s/padStart/padEnd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ops (this is padEnd, not padStart, but the same argument order applies)

@ljharb ljharb force-pushed the string_pad_start_end branch 2 times, most recently from e2da4c7 to 2a32b99 Compare March 30, 2016 21:42
@leobalter

Copy link
Copy Markdown
Member

LGTM :shipit:

@ljharb ljharb force-pushed the string_pad_start_end branch from 2a32b99 to 350f3d1 Compare April 2, 2016 16:47
@leobalter leobalter closed this in c95e673 Apr 6, 2016
@leobalter

Copy link
Copy Markdown
Member

Squashed and merged at c95e673, thanks!

@ljharb ljharb deleted the string_pad_start_end branch April 6, 2016 21:18
@ljharb

ljharb commented Apr 6, 2016

Copy link
Copy Markdown
Member Author

Thank you! :-D

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.

3 participants