Add tests for non-registered symbols in Weak{Map,Set,Ref} and FinalizationRegistry

[lionel-rowe]

I've added a testOk harness as similar try { testLogic(); return true } catch(e) { return { result: false, message: e.message } } logic seems to be repeated a lot, but I've only used it for the newly added tests for now.

[queengooborg]

Hey @lionel-rowe, thanks for doing this!

The changes make sense to me, but let's add the testOk helper in a separate PR and perform a complete migration, both to ensure that we've fully switched to it and to keep the diff clean! (Note: In the new helper function, we'll need to add a .catch() to promises as well.)

I also just noticed that your commits are unverified! It looks like the commits are signed, but could not be verified as GitHub does not have your public key. We require commits to be signed and verified -- would you mind configuring your signing key accordingly?

[lionel-rowe]

Hey @lionel-rowe, thanks for doing this!

The changes make sense to me, but let's add the testOk helper in a separate PR and perform a complete migration, both to ensure that we've fully switched to it and to keep the diff clean! (Note: In the new helper function, we'll need to add a .catch() to promises as well.)

Sure, makes sense — reverted for now. Re catch, I think supplying a second callback param to then will behave identically to .then(...).catch(...) as long as the happy-path callback is guaranteed not to throw?

I also just noticed that your commits are unverified! It looks like the commits are signed, but could not be verified as GitHub does not have your public key. We require commits to be signed and verified -- would you mind configuring your signing key accordingly?

Fixed that too 👍

[queengooborg]

Sure, makes sense — reverted for now. Re catch, I think supplying a second callback param to then will behave identically to .then(...).catch(...) as long as the happy-path callback is guaranteed not to throw?

Actually, no, the second argument handles if the promise was rejected, not if it failed due to an error. So, we want something like this: promise.then(success, fail).catch(fail)!

[lionel-rowe]

Actually, no, the second argument handles if the promise was rejected, not if it failed due to an error. So, we want something like this: promise.then(success, fail).catch(fail)!

At risk of derailing this PR, I don't think that's true. In the context of a promise, throwing and synchronously calling the reject callback do the same thing, and both are equally handled by the 2nd parameter to then vs the catch callback:

// in all of the below cases, console warns with `!`
// value of `results` is `{ status: 'fulfilled', value: undefined }` * 4
const results = await Promise.allSettled([
    new Promise(() => { throw '!' }).then(console.log, console.warn),
    new Promise((_, rej) => rej('!')).then(console.log, console.warn),
    new Promise(() => { throw '!' }).then(console.log).catch(console.warn),
    new Promise((_, rej) => rej('!')).then(console.log).catch(console.warn),
])

[queengooborg]

In regards to the changes performed in this PR, thank you very much for writing these tests, this is looking good to me! Welcome to the collector project!