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: private method usage: calling private method #1849

Closed

Conversation

jbhoosreddy
Copy link
Contributor

Tracking issue: #1343

Summary of changes:

  1. Add test for private method usage

Calling a private method

@jbhoosreddy
Copy link
Contributor Author

@leobalter @rwaldron This one needs more work on both frontmatter and coverage. But I wanted to get some initial 👀 on it.

  1. Can't find the normative spec text for the description in the spec.
  2. Also should coverage include all variants of private methods?

@jbhoosreddy
Copy link
Contributor Author

Also, wanted to use this space to discuss my next PR

Reading a private method separately from calling it

  • All instances of the class share the same function identity for the private method

Thoughts on testing function identity? I think I know it but just wanted to verify. cc @mkubilayk we can catch up about this tomorrow.

@rwaldron
Copy link
Contributor

If these are just valid syntax tests then there shouldn't be any assertions (there may be older tests that do this, but those just need to be split into "syntax only" and "runtime semantics only"). If you want to do runtime semantics tests that exercise all of the grammar variants, then you'll need to use src/class-elements/productions (template: productions).

@leobalter
Copy link
Member

I'm working on a fix for these cases.

The current cases are definitely not calling the presented assertions. They are also a simplified version of the rs-private-getter*.case / rs-private-method*.case files in the same folder.

@leobalter
Copy link
Member

fixed on #1919

@leobalter leobalter closed this Nov 5, 2018
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