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

Basic tests for WeakRef and FinalizationGroup #2192

Merged
merged 11 commits into from Jun 21, 2019

Conversation

@leobalter
Copy link
Member

commented Jun 3, 2019

The following is a checklist of basic observable points of the proposed specs, it doesn't go deep into abstractions and it should be good enough to track an initial set of tests.

Markdown is not amazing to handle the checkboxes, I won't spend much time fixing this, it's fair enough as it is.

The WeakRef Constructor
https://tc39.github.io/proposal-weakrefs/#sec-weak-ref-constructor

  • is the initial value of the WeakRef property of the global object.
  • is designed to be subclassable. It may be used as the value in an extends clause of a class definition. Subclass constructors that intend to inherit the specified WeakRef behaviour must include a super call to the WeakRef constructor to create and initialize the subclass instance with the internal state necessary to support the WeakRef.prototype built-in methods.

WeakRef ( target )
https://tc39.github.io/proposal-weakrefs/#sec-weak-ref-target

  1. If NewTarget is undefined, throw a TypeError exception.
  2. If Type(target) is not Object, throw a TypeError exception.
  3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget, "%WeakRefPrototype%", « [[Target]] »).
  4. Perfom ! KeepDuringJob(target).
  5. Set weakRef.[[Target]] to target.
  6. Return weakRef.
  • is extensible
  • [[proto]]

Properties of the WeakRef Constructor
https://tc39.github.io/proposal-weakrefs/#sec-properties-of-the-weak-ref-constructor

  • has a [[Prototype]] internal slot whose value is the intrinsic object %FunctionPrototype%.

WeakRef.prototype
https://tc39.github.io/proposal-weakrefs/#sec-weak-ref.prototype

  • This property has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }.

Properties of the WeakRef Prototype Object

  • has a [[Prototype]] internal slot whose value is the intrinsic object %ObjectPrototype%.
  • does not have a [[Target]] internal slot.

WeakRef.prototype.deref ( )
https://tc39.github.io/proposal-weakrefs/#sec-weak-ref.prototype.deref

  1. Let weakRef be the this value.
  2. If Type(weakRef) is not Object, throw a TypeError exception.
  3. If weakRef does not have a [[Target]] internal slot, throw a TypeError exception.
  4. Let target be the value of weakRef.[[Target]].
  5. If target is not empty,
    a. Perform ! KeepDuringJob(target).
    b. [ ] Return target.
  6. Return undefined.

WeakRef.prototype [ @@toStringTag ]
https://tc39.github.io/proposal-weakrefs/#sec-weak-ref.prototype-@@tostringtag

  • The initial value of the @@toStringTag property is the String value "WeakRef".
  • This property has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true }.

The FinalizationGroup Constructor
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group-constructor

  • is the initial value of the FinalizationGroup property of the global object.

FinalizationGroup ( cleanupCallback )
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group-cleanup-callback

  1. If NewTarget is undefined, throw a TypeError exception.
  2. If IsCallable(cleanupCallback) is false, throw a TypeError exception.
  3. Let finalizationGroup be ? OrdinaryCreateFromConstructor(NewTarget, "%FinalizationGroupPrototype%", « [[Realm]], [[CleanupCallback]], [[Cells]], [[IsFinalizationGroupCleanupJobActive]] »).
  4. Let fn be the active function object.
  5. Set finalizationGroup.[[Realm]] to fn.[[Realm]].
  6. Set finalizationGroup.[[CleanupCallback]] to cleanupCallback.
  7. Set finalizationGroup.[[Cells]] to be an empty List.
  8. Set finalizationGroup.[[IsFinalizationGroupCleanupJobActive]] to false.
  9. Return finalizationGroup.

Properties of the FinalizationGroup Constructor
https://tc39.github.io/proposal-weakrefs/#sec-properties-of-the-finalization-group-constructor

  • has a [[Prototype]] internal slot whose value is the intrinsic object %FunctionPrototype%.

FinalizationGroup.prototype
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype

  • This property has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }.

Properties of the FinalizationGroup Prototype Object
https://tc39.github.io/proposal-weakrefs/#sec-properties-of-the-finalization-group-prototype-object

  • has a [[Prototype]] internal slot whose value is the intrinsic object %ObjectPrototype%.
    is an ordinary object.
  • does not have a [[Target]] internal slot.

FinalizationGroup.prototype.constructor
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype.constructor

  • The initial value of FinalizationGroup.prototype.constructor is the intrinsic object %FinalizationGroup%.

FinalizationGroup.prototype.register ( target , holdings [, unregisterToken ] )
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype.register

  1. Let finalizationGroup be the this value.
  2. If Type(finalizationGroup) is not Object, throw a TypeError exception.
  3. If Type(target) is not Object, throw a TypeError exception.
  4. If finalizationGroup does not have a [[Cells]] internal slot, throw a TypeError exception.
  5. If Type(unregisterToken) is not Object,
    a. [ ] If unregisterToken is not undefined, throw a TypeError exception.
    b. [ ] Set unregisterToken to empty.
  6. Let cell be the Record { [[Target]] : target, [[Holdings]]: holdings, [[UnregisterToken]]: unregisterToken }.
  7. Append cell to finalizationGroup.[[Cells]].
  8. Return undefined.

FinalizationGroup.prototype.unregister ( unregisterToken )
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype.unregister

  1. Let finalizationGroup be the this value.
  2. If Type(finalizationGroup) is not Object, throw a TypeError exception.
  3. If finalizationGroup does not have a [[Cells]] internal slot, throw a TypeError exception.
  4. If Type(unregisterToken) is not Object, through a TypeError exception.
  5. Let removed be false.
  6. For each Record { [[Target]], [[Holdings]], [[UnregisterToken]] } cell that is an element of finalizationGroup.[[Cells]], do
    a. If SameValue(cell.[[UnregisterToken]], unregisterToken) is true, then
    i. Remove cell from finalizationGroup.[[Cells]].
    ii. Set removed to true.
  7. Return removed.

FinalizationGroup.prototype.cleanupSome ( [ callback ] )
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype.cleanupSome

  1. Let finalizationGroup be the this value.
  2. If Type(finalizationGroup) is not Object, throw a TypeError exception.
  3. If finalizationGroup does not have a [[Cells]] internal slot, throw a TypeError exception.
  4. If callback is not undefined and IsCallable(callback) is false, throw a TypeError exception.
  5. Perform ! CleanupFinalizationGroup(finalizationGroup, callback).
  6. Return undefined.

FinalizationGroup.prototype [ @@toStringTag ]
https://tc39.github.io/proposal-weakrefs/#sec-finalization-group.prototype-@@tostringtag

  • The initial value of the @@toStringTag property is the String value "FinalizationGroup".
  • This property has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true }.

CreateFinalizationGroupCleanupIterator ( finalizationGroup )
https://tc39.github.io/proposal-weakrefs/#sec-createfinalizationgroupcleanupiterator

  1. Let finalizationGroup be the this value.
  2. If Type(finalizationGroup) is not Object, throw a TypeError exception.
  3. If finalizationGroup does not have a [[Cells]] internal slot, throw a TypeError exception.
  4. Assert: realm.[[Intrinsics]].[[%FinalizationGroupCleanupIteratorPrototype%]] exists and has been initialized.
  5. Let prototype be finalizationGroup.[[Realm]].[[Intrinsics]].[[%FinalizationGroupCleanupIteratorPrototype%]].
  6. Let iterator be ObjectCreate(prototype, « [[FinalizationGroup]] »).
  7. Set iterator.[[FinalizationGroup]] to finalizationGroup.
  8. Return iterator.

The %FinalizationGroupCleanupIteratorPrototype% Object
https://tc39.github.io/proposal-weakrefs/#sec-%finalizationgroupcleanupiterator%-object

  • is an ordinary object.
  • has a [[Prototype]] internal slot whose value is the intrinsic object %IteratorPrototype%.

%FinalizationGroupCleanupIteratorPrototype%.next ( )
https://tc39.github.io/proposal-weakrefs/#sec-%finalizationgroupcleanupiterator%.next

  1. Let iterator be the this value.
  2. If Type(iterator) is not Object, throw a TypeError exception.
  3. If iterator does not have a [[FinalizationGroup]] internal slot, throw a TypeError exception.
  4. Let finalizationGroup be iterator.[[FinalizationGroup]].
  5. If finalizationGroup is undefined, then
    a. [ ] Return CreateIterResultObject(undefined, true).
  6. Assert: finalizationGroup has [[Cells]] and [[IsFinalizationGroupCleanupJobActive]] internal slot.
  7. If finalizationGroup.[[IsFinalizationGroupCleanupJobActive]] is false, throw a TypeError exception.
  8. If finalizationGroup.[[Cells]] contains a Record cell such that cell.[[Target]] is empty,
    a. Choose any such cell.
    b. Remove cell from finalizationGroup.[[Cells]].
    c. [ ] Return CreateIterResultObject(cell.[[Holdings]], false).
  9. Otherwise, return CreateIterResultObject(undefined, true).

%FinalizationGroupCleanupIteratorPrototype% [ @@toStringTag ]
https://tc39.github.io/proposal-weakrefs/#sec-%finalizationgroupcleanupiteratorprototype%-@@tostringtag

  • The initial value of the @@toStringTag property is the String value "FinalizationGroup Cleanup Iterator".
  • This property has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true }.

@leobalter leobalter self-assigned this Jun 3, 2019

@leobalter leobalter changed the title Basic tests for weakref and finalizationgroup [WIP] Basic tests for WeakRef and FinalizationGroup Jun 3, 2019

@leobalter leobalter force-pushed the leobalter:weakref branch 2 times, most recently from 9d1462e to d6505e9 Jun 3, 2019

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Failures from V8 with the --harmony-weak-refs flag:

FAIL test/built-ins/FinalizationGroup/newtarget-prototype-is-not-object.js
  newTarget.prototype is undefined Expected SameValue(«[object Object]», «[object FinalizationGroup]») to be true

FAIL test/built-ins/FinalizationGroup/proto-from-ctor-realm.js
  newTarget.prototype is undefined Expected SameValue(«[object Object]», «[object FinalizationGroup]») to be true

FAIL test/built-ins/WeakRef/newtarget-prototype-is-not-object.js
  newTarget.prototype is undefined Expected SameValue(«[object Object]», «[object WeakRef]») to be true

FAIL test/built-ins/WeakRef/proto-from-ctor-realm.js
  newTarget.prototype is undefined Expected SameValue(«[object Object]», «[object WeakRef]») to be true

These errors show the constructors are not setting the correct prototype if the NewTarget's prototype is not an object.

FAIL test/built-ins/WeakRef/prototype/constructor.js (default)
  Expected SameValue(«true», «false») to be true

FAIL test/built-ins/WeakRef/prototype/constructor.js (strict mode)
  Expected SameValue(«true», «false») to be true

This error relates to tc39/proposal-weakrefs#131

@mhofman

This comment has been minimized.

Copy link

commented Jun 4, 2019

FYI, V8 currently implements an older version of the spec, and even then has deficiencies against that. See https://bugs.chromium.org/p/v8/issues/detail?id=8179#c24 and further.

Edit: After some spec updates, the main behavioral ones are that unregister doesn't return a boolean and that the callback argument to cleanupSome is ignored.

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

FYI, V8 currently implements an older version of the spec

Sure, I only use any test results as informative data, the tests are against the specs, which work as the final source of truth for each assertion.

The tests may become useful to fix the implementation after the updates.

After some spec updates, the main behavioral ones are that unregister doesn't return a boolean and that the callback argument to cleanupSome is ignored.

I noticed that, and therefore we have tests for it as well.

@rwaldron
Copy link
Contributor

left a comment

Cursory review of the FinalizationGroup tests is done. I haven't looked at WeakRef yet.

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Wow, I appreciate the review! I'll get them addressed tomorrow.


@mhofman @gsathya do you suggest any strategy to empty the [[Target]] from WeakRefs? Something that could work from cross browsers? I might end up in need to force an extension to $262.

@gsathya

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@mhofman @gsathya do you suggest any strategy to empty the [[Target]] from WeakRefs? Something that could work from cross browsers? I might end up in need to force an extension to $262.

d8 exposes a gc function with a flag (--expose-gc) that triggers the gc, so I think adding an extension to $262 to expose this function would probably be the best way to make the tests reliable. This is what we use in our v8 tests for weakrefs: https://cs.chromium.org/chromium/src/v8/test/mjsunit/harmony/weakrefs/unregister-before-cleanup.js?l=29&rcl=7b60ccba767a2cde33badc5c35e05e67e8723275

@mhofman

This comment has been minimized.

Copy link

commented Jun 11, 2019

@mhofman @gsathya do you suggest any strategy to empty the [[Target]] from WeakRefs? Something that could work from cross browsers? I might end up in need to force an extension to $262.

d8 exposes a gc function with a flag (--expose-gc) that triggers the gc, so I think adding an extension to $262 to expose this flag would probably be the best way to make the tests reliable.

I'm not sure how spidermonkey is invoked for the tests but by default jsshell exposes a gc function that also triggers the gc.

The interesting part in testing is in dealing with the interaction of jobs scheduling and garbage collection. If the WeakRef was just created or derefed in the same execution, the [[Target]] object was added to the keep alive list. Then of course, you also have to make sure the [[Target]] object is in fact collectible, not just that it cannot be "observably referenced", but also sometimes work around some engine gc limitations.

If it helps, I've created a helper that allows me to await until an object was collected: https://github.com/mhofman/tc39-weakrefs-shim/blob/81645e4e96042f6e35bd6805d36f93db9ee46f5e/tests/collector-helper.ts#L18-L50
It relies on creating a new FinalizationGroup instance and a gc() function.

@gsathya

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

The interesting part in testing is in dealing with the interaction of jobs scheduling and garbage collection. If the WeakRef was just created or derefed in the same execution, the [[Target]] object was added to the keep alive list. Then of course, you also have to make sure the [[Target]] object is in fact collectible, not just that it cannot be "observably referenced", but also sometimes work around some engine gc limitations.

Looks like you're relying on setImmediate to make sure the task is completed, but that's not part of the ES spec.

I don't know if test262 wants tests that extend beyond the ES spec -- it might be good enough to just check that the finalizer has not run when we get to the microtask queue.

@mhofman

This comment has been minimized.

Copy link

commented Jun 12, 2019

The interesting part in testing is in dealing with the interaction of jobs scheduling and garbage collection.

Looks like you're relying on setImmediate to make sure the task is completed, but that's not part of the ES spec.

I don't know if test262 wants tests that extend beyond the ES spec -- it might be good enough to just check that the finalizer has not run when we get to the microtask queue.

Right, that's one of the challenges.
The spec defers the scheduling of the ClearKeptObjects and CleanupFinalizationGroup abstract operations to the embedding environment.
I'm not sure what's the correct way to trigger those from tests, or at least make sure that execution point was crossed.

The tests in my shim rely on setImmediate (internally polyfilled) because that's what the shim itself uses. It's the closest I could get to the microtask checkpoint that browser embedders use.

I believe jsshell doesn't have a notion of task or full event loop. Only a simple job queue. I'm not sure what's the plan for the weakref integration. In my shim I simply expect the program to setup its own event-loop and provide a setImmediate (which my tests do).

@gsathya, how does d8 work there? Are the ClearKeptObjects and CleanupFinalizationGroup operations internally triggered if not embedded? In your tests I see you use setTimeout and %PerformMicrotaskCheckpoint. Does that mean v8 itself does have a notion of event-loop and full tasks?

I think we need to find a solution here. Just testing the "was not collected in the same synchronous execution" part of the spec is not sufficient.
I guess with cleanupSome you can test some of the collection behavior without relying on host job integration. However you can't test the behavior of the embedder invoking the default callback on its own job, especially making sure exceptions are not swallowed. And of course, you can't check that WeakRef ever releases the target.

@littledan, do you have any suggestions on how to test the parts of the spec that require execution of the host hooks?

@gsathya

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@gsathya, how does d8 work there? Are the ClearKeptObjects and CleanupFinalizationGroup operations internally triggered if not embedded? In your tests I see you use setTimeout and %PerformMicrotaskCheckpoint. Does that mean v8 itself does have a notion of event-loop and full tasks?

d8 does have a full event loop, so I plan on adding these host operations to d8 and test it. The current WeakRef implementation in v8 is based on the old semantics of running the cleanup callback in the microtask, but I'm changing it right now.

I think we need to find a solution here. Just testing the "was not collected in the same synchronous execution" part of the spec is not sufficient.

I don't think test262 needs to find a solution. Embedders are free to do whatever here as it's not mandated by the spec. Note that non collecting embedders are spec compliant, so they shouldn't fail any test262 tests.

I wonder if these tests belong in Web platform tests instead as they test the spec defined by WHATWG/HTML.

@mhofman

This comment has been minimized.

Copy link

commented Jun 13, 2019

I don't think test262 needs to find a solution. Embedders are free to do whatever here as it's not mandated by the spec. Note that non collecting embedders are spec compliant, so they shouldn't fail any test262 tests.

I wonder if these tests belong in Web platform tests instead as they test the spec defined by WHATWG/HTML.

Right, but how should test262 go about testing the behavior of iterators and other parts that are only accessible after collection has occurred? For example the guard to make sure iterators are not used asynchronously. Those definitely belong here.
While a non collecting engine is compliant, a collecting one could be tested to be compliant on those steps only accessible after collection?

@gsathya

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I'm fine with deferring all that to web platform tests as well. What has test262 done for other language features that required extensive embedder hooks?

@mhofman

This comment has been minimized.

Copy link

commented Jun 14, 2019

From a cursory look, the atomics seem to have some agent integration with yield and sleep: #1617

Maybe a related $262.agent.tryTask() that would go through a new task?

And if gc is available, a $262.agent.tryGC() function that would enable testing behavior accessible after collection?

Looking at the spec, another area that relies on host hooks is module import with the HostResolveImportedModule abstract operation.
I'm guessing the tests assume the basic relative path resolution behavior?

@littledan

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

As @mhofman says, test262 has lots of hooks that can only be implemented by particular embedders, for example, to detach an ArrayBuffer. I think it would be possible to find similar hooks that would let us test WeakRefs in test262. We could also use a feature flag to indicate that certain HTML-analogous behavior is required (and turn off the test for environments which differ). If we can't figure out an acceptable way to reach full test coverage within test262, we could write web-platform-tests and other host-level test suites to fill the gaps.

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@littledan:

I think it would be possible to find similar hooks that would let us test WeakRefs in test262.

I'm up for this, as it matches behavior we already have in the specs. the $262 api + eshost combo is the best hook we have today and we should keep this strategy.

@mhofman:

I'm guessing the tests assume the basic relative path resolution behavior?

It's specified in the documentation for Test262:
https://github.com/tc39/test262/blob/master/INTERPRETING.md#modules

Maybe a related $262.agent.tryTask() that would go through a new task?

And if gc is available, a $262.agent.tryGC() function that would enable testing behavior accessible after collection?

yes, I've been coordinating this work with @rwaldron to have a proper hook in eshost and make sure $262 is reusable in many implementations.

@gsathya:

I'm fine with deferring all that to web platform tests as well.

I'd like to experiment the hooks here to make sure we caught everything properly. WPT can be useful with perhaps some direct GC, but I'd like to see weakrefs being well implemented in other places like node and embedded js iot. It's such a fundamental piece of weakrefs and I'm really excited to have it.

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

From my perspective, most of the next tests depends on a GC hook. I'd like to stop this work here and continue from a follow up PR. These can be called the basic tests.

This should minimize review burnout for everyone.

Anyone up to review the current coverage?

@leobalter leobalter changed the title [WIP] Basic tests for WeakRef and FinalizationGroup Basic tests for WeakRef and FinalizationGroup Jun 19, 2019

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

I actually missed uploading the review addressing for #2192 (review)

@gsathya

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I'd like to experiment the hooks here to make sure we caught everything properly. WPT can be useful with perhaps some direct GC, but I'd like to see weakrefs being well implemented in other places like node and embedded js iot. It's such a fundamental piece of weakrefs and I'm really excited to have it.

SGTM. I'm happy to add hooks to d8

From my perspective, most of the next tests depends on a GC hook. I'd like to stop this work here and continue from a follow up PR. These can be called the basic tests.

SGTM.

Anyone up to review the current coverage?

@mhofman knows the spec best, it'd be great to get their review. @wingo, maybe too?

@littledan
Copy link
Member

left a comment

This looks like a correct set of tests for the API surface.

Seems like it's a lot of work to write all of these repetitive tests! I wonder if a harness analogous to idlharness.js could be useful for test262 to reduce this load in the future.

I'm looking forward to the second set of tests which gets at the core of WeakRefs, but I see no problem with landing this first set of tests earlier.

Show resolved Hide resolved .../FinalizationGroup/prototype/cleanupSome/callback-not-callable-throws.js

var actual = WeakRef.prototype.hasOwnProperty('constructor');

// If implemented, it should conform to the spec text

This comment has been minimized.

Copy link
@littledan

littledan Jun 20, 2019

Member

This test should be written in a way that engines which implement Annex B can reliably assert that the property exists. The previous directory structure provided a way to do this; we shouldn't regress just because normative optional is inline. Maybe normative-optional could be a feature flag.

This comment has been minimized.

Copy link
@leobalter

leobalter Jun 20, 2019

Author Member

this flag would make the tests results rely more on how the metadata is interpreted and this needs some coordination with people who maintain test runners. Otherwise the js code would just straight forward force a feature.

I'd prefer to discuss this in a follow up as well, if you don't mind.

@rwaldron
Copy link
Contributor

left a comment

Minor changes and then I can land it

@rwaldron

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@littledan I'm not sure why I can't reply to your comment, so I'm replying here...

I don't understand what these comments are getting at. I do see cleanupSome used in this test file.

You are seeing the updated file, but for whatever reason github didn't auto resolve my comments. Had you reviewed the commits, you'd see that @leobalter fixed that file: c6aadc7#diff-058bfda97628c83bd0f48b02903b9229

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

I think that's related to getting the false positive. This assertion is there to prevent the tests from false positives. like:

var obj = {};
/* ... */
assert.sameValue(typeof obj.foo, 'function');

assert.throws(TypeError, function() {
  obj.foo(42);
}, 'foo throws if argument is 42');

// The assertion above would also throw a TypeError if foo is not a function
// The test would pass with foo not being implemented.

This is a recent style we've been trying to use preventing tests that pass before a feature is implemented.

@leobalter leobalter dismissed stale reviews from rwaldron and littledan via 1a24784 Jun 21, 2019

@leobalter leobalter force-pushed the leobalter:weakref branch from c6aadc7 to 1a24784 Jun 21, 2019

@rwaldron rwaldron merged commit f7591eb into tc39:master Jun 21, 2019

1 of 7 checks passed

ci/circleci: ChakraCore: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: JSC: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: SpiderMonkey: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: V8 --harmony: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: V8: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: XS: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: Project lint, generation tests and build Your tests passed on CircleCI!
Details
@mhofman
Copy link

left a comment

@mhofman knows the spec best, it'd be great to get their review.

Sorry for the late review.

I'm wondering, should there be a specific test to make sure WeakRef and FinalizationGroup are subclassable?
The specs mentions that aspect, but I'm not sure if that's something that is usually tested explicitly.


/*---
esid: sec-finalization-group.prototype.register
description: Throws a TypeError if this does not have a [[Cells]] internal slot

This comment has been minimized.

Copy link
@mhofman

mhofman Jun 21, 2019

Small nitpick, but the test's file name mentions target not cells.

assert.throws(TypeError, function() {
register.call({ ['[[Cells]]']: {} }, target);
}, 'Ordinary object without [[Cells]]');

This comment has been minimized.

Copy link
@mhofman

mhofman Jun 21, 2019

Thinking that FinalizationGroup.prototype probably shouldn't have a [[Cells]] internal slot.
Opened tc39/proposal-weakrefs#143

@leobalter leobalter deleted the leobalter:weakref branch Jun 21, 2019

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

I'm wondering, should there be a specific test to make sure WeakRef and FinalizationGroup are subclassable?

I plan to have these tests, but they are low priority in the given checklist. It should be pretty easy to do it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.