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

[DON'T SQUASH] Update params trailing comma tests #911

Merged
merged 10 commits into from
Mar 14, 2017

Conversation

leobalter
Copy link
Member

Closes #822

@leobalter
Copy link
Member Author

cc @littledan

@leobalter leobalter self-assigned this Mar 13, 2017
leobalter referenced this pull request in rwaldron/test262 Mar 13, 2017
…mma-arguments.js. Fixes tc39gh-822

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
@leobalter
Copy link
Member Author

these tests are invalid. I'm fixing this PR soon

function* f1() {
    length = arguments.length;
};
f1(1,).next();

this has no connection with the trailing commas on the parameters list, but the arguments object and how the length is formed to correspond the given arguments list.

Instead of adding this extra case, I'm removing all the params-trailing-comma-arguments.js files.

@leobalter
Copy link
Member Author

In the meanwhile, I need to search for tests asserting the well formed arguments.length

@leobalter
Copy link
Member Author

test/language/expressions/call/trailing-comma.js is the only other test for trailing comma in a call expression, but there's no tests for the arguments.length

@leobalter leobalter changed the title Add test for arguments length in a generator fn w/ trailing comma [DON'T SQUASH] Update params trailing comma tests Mar 14, 2017
@leobalter
Copy link
Member Author

I can add more cases but that would create more and more files and it's becoming way larger on every new addition.

I suggest we follow up with new PRs to have easier reviews.

Copy link
Contributor

@gnarf gnarf left a comment

Choose a reason for hiding this comment

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

This is some pretty dense stuff, but I read over all the .template and .case files, nothing stands out to me, the info blocks seem to match up, etc.

I'd still prefer if @littledan, @rwaldron, or someone else would take a good look here (even if after we merge it)

@leobalter
Copy link
Member Author

these tests will open the way for me to add more stuff on async generators, early errors, etc. I'll have this merged but already assuming the responsibility to fix anything else we find wrong.

@leobalter leobalter merged commit f17cd6d into tc39:master Mar 14, 2017
@leobalter leobalter deleted the 822-missing-args-length branch March 14, 2017 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants