-
Notifications
You must be signed in to change notification settings - Fork 479
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
private elements: tests for private method semantics usage #1935
private elements: tests for private method semantics usage #1935
Conversation
@mkubilayk for initial review |
} | ||
|
||
const c1 = new C(); | ||
const c2 = new C(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This renders inside the class body:
var C = class {
*m() { return 42; } #m() {};
method() {
return this.#m;
}
const c1 = new C();
const c2 = new C();
}
#IdentifierName | ||
Return the String value consisting of the sequence of code units corresponding to PrivateName | ||
|
||
template: productions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the "default" templates, as "productions" will render tests that will contain a bunch of unnecessary and irrelevant code.
#IdentifierName | ||
Return the String value consisting of the sequence of code units corresponding to PrivateName | ||
|
||
template: productions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in the previous review comment, please use the "default" template. This will produce a test that looks like this:
var C = class {
static m() { return 42; } #foo() {};
method() {
return this.#foo;
}
}
var c = new C();
assert.sameValue(C.m(), 42);
assert.sameValue(Object.hasOwnProperty.call(c, "m"), false);
assert.sameValue(Object.hasOwnProperty.call(C.prototype, "m"), false);
verifyProperty(C, "m", {
enumerable: false,
configurable: true,
writable: true,
});
assert.sameValue(c.method().name, '#foo');
When it really only needs to produce a test that looks like this:
var C = class {
#foo() {};
method() {
return this.#foo;
}
}
var c = new C();
assert.sameValue(c.method().name, '#foo');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests need a bit more work. It also needs some other fixings as pointed by @rwaldron
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
desc: PrivateName IdentifierName PropName StringValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description should tell what's being verified in this test, I can't read this and tell anything.
Static Semantics: PrivateStringValue | ||
PrivateName:: | ||
#IdentifierName | ||
Return the String value consisting of the sequence of code units corresponding to PrivateName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text doesn't match the test contents
static #bar() { return '262'; } | ||
//- privateinspectionfunctions | ||
method1() { | ||
return this.#foo.call(C); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs adding the matching reference from the spec that allows this in the desc/info above. Otherwise it's hard to tell the goal of this test. The top comment on this PR clarified it a bit for me.
This needs adding a group of tests that assert other behaviors:
- when the function is called with other values, like
this.#foo.call(this)
,this.#foo.call({})
,this.#foo.call(null)
,C.#bar.call(C)
, including other values as the instance variable name (instead ofthis
),undefined
, etc etc - other possible private methods.
- fields with function values, perhaps?
//- privateinspectionfunctions | ||
method() { | ||
return this.#m; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can (and should) also be verified for other methods types including assessor methods
Static Semantics: PrivateStringValue | ||
PrivateName:: | ||
#IdentifierName | ||
Return the String value consisting of the sequence of code units corresponding to PrivateName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also not testing the name property
#m() {} | ||
//- privateinspectionfunctions | ||
method() { | ||
return this.#m; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we already have tests to assert the method relationship with the class prototype? In this case, it should be good to assert which object has their own property and the shared relationship.
thanks for the review. making revisions now. |
I'm temporarily closing this.. need some time to look at it. |
Tracking issue: #1343
Summary of changes: