Skip to content
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

Tests for NumericSeparatorLiteral. Closes gh-1051 #1176

Merged
merged 3 commits into from Aug 21, 2017

Conversation

@rwaldron
Copy link
Contributor

commented Aug 14, 2017

No description provided.

@leobalter
Copy link
Member

left a comment

Just a few comments. The tests are good!

I love the file names, it took me less than a minute to read them properly.

One thing I imagined for this is something checking whitespaces including line terminators, like:

1
_
1

on this case, the return of that - in an expression - is 1, and we can have _ with a defined accessor desc to verify the access.

what do you think? too weird?

assert.sameValue(0b0_1, 0b01);
assert.sameValue(0b0_1, 0b01);
assert.sameValue(0b0_1, 0b01);
assert.sameValue(0b0_1, 0b01);

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

stress tests? :)

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 14, 2017

Author Contributor

lol @ me

---*/

assert.sameValue(0b0_1, 0b01);

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

isn't this the same as in test/language/literals/numeric/numeric-separator-literal-bil-bd-nsl-bd-one-of.js?

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 14, 2017

Author Contributor

👍

assert.sameValue(1_8, 18);
assert.sameValue(1_9, 19);


This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

while you're fixing the other parts, you may want to remove this extra line.

---*/

assert.sameValue(1_0, 10);

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

this case already exists in other files. I don't mind, but if the idea is testing one of, it would nice to add more assertions in this same file for:

2_1, 2_3, 2_4...
3_1, 3_2, 3_4...
...

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 14, 2017

Author Contributor

No thanks ❤️

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

you're welcome ❤️

@rwaldron rwaldron force-pushed the rwaldron:1051 branch from fcb79ef to 4938f1d Aug 14, 2017


/*---
esid: prod-NumericLiteralSeparator
description: description: NumericLiteralSeparator may not appear adjacent to another NumericLiteralSeparator in a BinaryIntegerLiteral

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

description description


/*---
esid: prod-NumericLiteralSeparator
description: `.` DecimalDigit NumericLiteralSeparator DecimalDigit ExponentPart_opt

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

wrap the value with quotes, otherwise the yaml will fail parsing

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

same for other cases here with

description: `

/*---
esid: prod-NumericLiteralSeparator
description: description: NumericLiteralSeparator may not appear adjacent to another NumericLiteralSeparator in a OctalIntegerLiteral

This comment has been minimized.

Copy link
@leobalter

leobalter Aug 14, 2017

Member

this is a profound description

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 15, 2017

Author Contributor

lol

@rwaldron rwaldron force-pushed the rwaldron:1051 branch 2 times, most recently from d6f022f to e6b429c Aug 15, 2017

@rwaldron rwaldron force-pushed the rwaldron:1051 branch from e120702 to 67970f0 Aug 17, 2017

@leobalter

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

We've got a few frontmatter linting bugs.

test/built-ins/parseFloat/tonumber-numeric-separator-literal-bil-bd-nsl-bd.js: FRONTMATTER - No valid YAML-formatted frontmatter
test/built-ins/parseFloat/tonumber-numeric-separator-literal-hil-hd-nsl-hd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dd-dot-dd-ep-sign-plus-dd-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-oil-ods-nsl-od.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-hil-hds-nsl-hd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-nzd-nsl-dd-one-of.js: FRONTMATTER - No valid YAML-formatted frontmatter
test/built-ins/parseFloat/tonumber-numeric-separator-literal-oil-ods-nsl-ods.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-oil-od-nsl-od.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dot-dds-nsl-dd-ep.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-sign-minus-dds-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-oil-od-nsl-ods.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dot-dd-nsl-dds-ep.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-nzd-nsl-dds.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dd-dot-dd-ep-sign-plus-dds-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-hil-od-nsl-od-one-of.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dd-dot-dd-ep-sign-minus-dds-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-bil-bd-nsl-bds.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-bil-bds-nsl-bds.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-sign-plus-dds-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-hil-hd-nsl-hds.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-nzd-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dd-nsl-dd-one-of.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dds-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dot-dds-nsl-dds-ep.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dds-dot-dd-nsl-dd-ep-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-oil-od-nsl-od-one-of.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dot-dd-nsl-dd-ep.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-dd-dot-dd-ep-sign-minus-dd-nsl-dd.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-hil-hds-nsl-hds.js: FRONTMATTER - Required fields missing: description
test/built-ins/parseFloat/tonumber-numeric-separator-literal-bil-bds-nsl-bd.js: FRONTMATTER - Required fields missing: description
@leobalter

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

Thanks for adding the stage 3 label, I thought it was already on Stage 3 and almost merged this.

@rwaldron rwaldron force-pushed the rwaldron:1051 branch from 67970f0 to 3dd5018 Aug 21, 2017

@rwaldron

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2017

Fixes to frontmatter pushed

@leobalter leobalter merged commit e88fea4 into tc39:master Aug 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leobalter

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

@rwaldron can you open a new PR with these commits? I clicked the merge button too early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.