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

test: Add test for private method in object literal #1760

Closed

Conversation

jbhoosreddy
Copy link
Contributor

Tracking Issue: #1343

Summary of changes:

  • Add tests for private method in object literal

@jbhoosreddy
Copy link
Contributor Author

@rwaldron @leobalter for review

not really sure if that's the right place to even put these tests. thoughts?

@rwaldron
Copy link
Contributor

These need to be moved:

test/language/literals/object/ => test/language/expressions/object/method-definition/

@jbhoosreddy
Copy link
Contributor Author

@rwaldron for review

@rwaldron
Copy link
Contributor

I want to think more about this changeset. I have two concerns:

  1. I want to welcome your contribution, because you put in the work and that should be recognized. However...
  2. It occurred to me that tests like this are generally avoided because it's hard to tell where the "coverage boundary" begins and ends, ie. what part of the spec is this actually testing? The obvious answer is that it's testing to ensure that PrivateName is not allowed inside PropertyDefinitionList—but I'd counter that a lot of things aren't allowed inside PropertyDefinitionList... for example, Python code (that's a bit extreme, but I think it illustrates well).

Let's think about this a bit more.

@jbhoosreddy
Copy link
Contributor Author

jbhoosreddy commented Sep 20, 2018

@rwaldron I don't mind scrapping this PR. If we decide we don't want to cover these, do you mind updating the test plan linked to the PR?
Edit: cc @littledan also

@littledan
Copy link
Member

We previously considered such "private name shorthand", e.g., in https://github.com/littledan/proposal-private-shorthand , and decided to break it out into a separate, follow-on proposal. I think it's worth it to have a test that it is not supported, given that history. (Related: #1782 (comment))

@jbhoosreddy
Copy link
Contributor Author

similar to #1782. closing

@littledan
Copy link
Member

Note, the lack of these tests led to a delay in discovering a real issue in the V8 implementation of private fields and methods: https://chromium-review.googlesource.com/c/v8/v8/+/1461161 . I'd ask you to reconsider whether such tests are OK. Note that the recently committed super.#x tests are a similarly specified case of the grammar not having a particular production for it, not an early error. cc @gsathya

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