Add tests for Stage 3 Decorator Metadata#3971
Conversation
rbuckton
force-pushed
the
decorator-metadata
branch
from
fca9a74 to
d615112
Compare
rbuckton
force-pushed
the
decorator-metadata
branch
from
d615112 to
ae45c3d
Compare
test/language/expressions/class/decorator/metadata/context-metadata-prop-desc.js
Outdated
Show resolved
Hide resolved
test/language/expressions/class/decorator/metadata/metadata-attached-when-class-is-decorated.js
Show resolved
Hide resolved
test/language/expressions/class/decorator/metadata/metadata-prop-desc.js
Outdated
Show resolved
Hide resolved
test/language/statements/class/decorator/metadata/context-metadata-prop-desc.js
Outdated
Show resolved
Hide resolved
test/language/statements/class/decorator/metadata/metadata-attached-when-class-is-decorated.js
Show resolved
Hide resolved
test/language/statements/class/decorator/metadata/metadata-prop-desc.js
Outdated
Show resolved
Hide resolved
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
seems good! would be nice to have another review before merging.
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Thanks for doing this!
The only blocker would be the context metadata property descriptor (if I'm correct about that.) I had a few other suggestions for improvements elsewhere.
Note that all the comments on expressions/class/decorator/metadata/ apply to the corresponding tests in statements/class/decorator/metadata/ as well.
| assert.deepEqual(kinds, { | ||
| "class": true, | ||
| "public method": true, | ||
| "public getter": true, | ||
| "public setter": true, | ||
| "public field": true, | ||
| "public accessor": true, | ||
| "private method": true, | ||
| "private getter": true, | ||
| "private setter": true, | ||
| "private field": true, | ||
| "private accessor": true, | ||
| }); |
There was a problem hiding this comment.
We're trying to get rid of assert.deepEqual (#3476). Maybe assert.compareArray(Object.entries(kinds), [...]) would work here.
| /*--- | ||
| esid: sec-createdecoratorcontextobject | ||
| description: > | ||
| Property descriptor for metadata property of decorator context object. |
There was a problem hiding this comment.
The test doesn't seem to be about the property descriptor, maybe this was accidentally copied from another test?
There was a problem hiding this comment.
One thing this test could additionally do that it doesn't currently do: make sure the private/kind metadata for each invocation of the decorator matches the type of thing that it decorates. Currently the test passes if each private/kind combination is hit once, but an implementation could switch two of them (e.g., private accessors are reported as public and vice versa) and the test would still pass.
(May or may not be worth it.)
| /*--- | ||
| esid: sec-runtime-semantics-classdefinitionevaluation | ||
| description: > | ||
| Metadata on a derived class inherits from the metadata of the declared super class. |
There was a problem hiding this comment.
Suggestion, the description could be a bit more specific here, more like the title of the test: maybe something like "Metadata on a derived class inherits from the original metadata of the declared super class, unaffected by overwriting"
| /*--- | ||
| esid: sec-runtime-semantics-classdefinitionevaluation | ||
| description: > | ||
| Metadata is only attached when a class or class element is decorated. |
There was a problem hiding this comment.
This description seems copied from somewhere else, maybe something like "Metadata inherits from the metadata of the super class"?
|
|
||
| let C = @dec class C {}; | ||
| const metadata = C[Symbol.metadata]; | ||
| assert(Object.isExtensible(metadata)); |
There was a problem hiding this comment.
| assert(Object.isExtensible(metadata)); | |
| assert(Object.isExtensible(metadata), "Metadata is not frozen"); |
(I suggest using assertion messages at least in plain assert(), otherwise if it fails you get "Expected false to be true" or something like that)
There was a problem hiding this comment.
Should we additionally verify that the object is a plain object, something like this?
assert.sameValue(Object.getPrototypeOf(metadata), Object.prototype, "Metadata's prototype is Object.prototype");| void @dec class C {}; | ||
| assert.sameValue(typeof contextObj.metadata, "object"); | ||
| verifyProperty(contextObj, 'metadata', { | ||
| enumerable: false, |
There was a problem hiding this comment.
By my reading of the spec, this seems like it should be true? In CreateDecoratorContextObject we create the property with CreateDataPropertyOrThrow, which calls CreateDataProperty, which creates enumerable properties.
3 participants
This adds tests for the Stage 3 Decorator Metadata proposal, per the specification PR at pzuraq/ecma262#10.
/cc: @pzuraq