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

test: add test cases to test-marshal.js for records #2306

Merged
merged 3 commits into from
Feb 15, 2021
Merged

Conversation

warner
Copy link
Member

@warner warner commented Feb 1, 2021

This exercises the current behavior of records, both empty objects (with and
without a Far marking), and objects that contain data and/or methods.

It also checks that Far/Remotable can be serialized and passes the
mustPassByPresence() check.

first step of #2018

@warner warner added the marshal package: marshal label Feb 1, 2021
@warner warner requested a review from erights February 1, 2021 08:47
@warner warner self-assigned this Feb 1, 2021
@michaelfig
Copy link
Member

It also checks that Far/Remotable can be serialized and passes the
mustPassByPresence() check.

Why does it do that? The point of the WeakMap is to guarantee that Remotables don't need to be further checked: if they are marked, they are handled specially.

@michaelfig
Copy link
Member

It also checks that Far/Remotable can be serialized and passes the mustPassByPresence() check.

If you want to get rid of the special handling and explicitly check the Remotable's contents every time with code, you'll have to avoid adding the prototype to a Remotable. Please don't special-case prototypes, Symbol.toStringTag or toString.

That will be a regression in the debugging experience, and the REPL will have to change as well to print Remotables better.

@warner
Copy link
Member Author

warner commented Feb 4, 2021

Given the understanding achieved today in #2018 (comment) , I'll change this to remove the mustPassByReference(remotable) tests, as the current code does not attempt to make that claim (even though liveslots was assuming it did). That test can return when I make the changes described in the 2018 comment, and we stop using an inserted prototype to provide the debugging information.

@warner warner force-pushed the 2018-add-tests branch 2 times, most recently from f40c2d6 to 2e22da2 Compare February 4, 2021 23:47
warner and others added 2 commits February 14, 2021 17:07
This exercises the current behavior of records, both empty objects (with and
without a Far marking), and objects that contain data and/or methods.

It also checks that Far/Remotable can be serialized.
@erights
Copy link
Member

erights commented Feb 15, 2021

Hi @warner I reassigned this to me and just reconciles with #2361 . I cannot add you as a reviewer but please review anyway. Thanks.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.

@erights erights merged commit 4b2fe4f into master Feb 15, 2021
@erights erights deleted the 2018-add-tests branch February 15, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
marshal package: marshal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants