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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

private elements: tests for private method semantics usage #1935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/class-elements/private-method-multiple-instance-identity-same.case
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright (C) 2018 Jaideep Bhoosreddy (Bloomberg LP). All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.

/*---
desc: Multiple class instances of PrivateName Private method have same function identity
info: |
Static Semantics: PrivateStringValue
PrivateName::
#IdentifierName
Return the String value consisting of the sequence of code units corresponding to PrivateName
Copy link
Member

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


template: productions
Copy link
Contributor

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.

features: [class-methods-private]
---*/

//- fields
#m() {}
//- privateinspectionfunctions
method() {
return this.#m;
Copy link
Member

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.

}
Copy link
Member

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


const c1 = new C();
const c2 = new C();
Copy link
Contributor

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();

}


//- assertions
assert.sameValue(c1.method(), c2.method());
23 changes: 23 additions & 0 deletions src/class-elements/private-method-name-property.case
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Copyright (C) 2018 Jaideep Bhoosreddy (Bloomberg LP). All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.

/*---
desc: PrivateName IdentifierName PropName StringValue
info: |
Static Semantics: PrivateStringValue
PrivateName::
#IdentifierName
Return the String value consisting of the sequence of code units corresponding to PrivateName

template: productions
Copy link
Contributor

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');

features: [class-methods-private]
---*/

//- fields
#foo() {}
//- privateinspectionfunctions
method() {
return this.#foo;
}
//- assertions
assert.sameValue(c.method().name, '#foo');
28 changes: 28 additions & 0 deletions src/class-elements/private-static-method-function-call-access.case
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (C) 2018 Jaideep Bhoosreddy (Bloomberg LP). All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.

/*---
desc: PrivateName IdentifierName PropName StringValue
Copy link
Member

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.

info: |
Static Semantics: PrivateStringValue
PrivateName::
#IdentifierName
Return the String value consisting of the sequence of code units corresponding to PrivateName
Copy link
Member

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


template: productions
features: [class-methods-private]
---*/

//- fields
#foo() { return 'test'; }
static #bar() { return '262'; }
//- privateinspectionfunctions
method1() {
return this.#foo.call(C);
Copy link
Member

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 of this), undefined, etc etc
  • other possible private methods.
  • fields with function values, perhaps?

}
method2() {
return C.#bar.call(this);
}
//- assertions
assert.sameValue(c.method1(), 'test');
assert.sameValue(c.method2(), '262');