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

refactor(swingset-liveslots): rep vs instance label tests more similar #7419

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

erights
Copy link
Member

@erights erights commented Apr 14, 2023

Improve the parallelism of heap instance labeling test vs representative labeling test. In particular, add a passStyleOf tests that had failed before #7273

@erights erights requested a review from warner April 14, 2023 14:06
@erights erights self-assigned this Apr 14, 2023
@erights erights force-pushed the markm-rep-label-like-instance-label branch 4 times, most recently from 06a79ae to 5fcbdc8 Compare April 14, 2023 22:26
@erights
Copy link
Member Author

erights commented Apr 14, 2023

I wrote the following text before #7273 and am preserving it as a historical note.

After #7410 was merged, I suddenly wondered why @warner 's added test of representative labeling seemed to work prior to the endo reconciliation. By contrast, I had to suppress the similar tests in test-label-instance.js with a test.skip, because until the reconciliation, the new @@toStringTag data property violates the remotable invariants. Thus, prior to the reconciliation, the labeled instances or representatives are not even Passable. passStyleOf on them throws.

The reason @warner 's test passes anyway is that it doesn't do a passStyleOf, or indeed anything that is actually sensitive to whether these allegedly virtual objects are actually Passable. Frankly, I'm surprised that it was able to get that far. (@warner @FUDCo , Perhaps that indicates we need an additional validation test somewhere?)

In any case, this PR adds the passStyleOf tests that should be there, restoring the parallelism with the instance labeling test. Doing so did cause the expected failure. Good! So the price is that we needed to mark these as test.skip, just as we had to do with the instance labeling tests, with a TODO to unskip following the reconciliation.

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

LGTM

@erights erights force-pushed the markm-rep-label-like-instance-label branch from 5fcbdc8 to 262384a Compare April 15, 2023 03:17
@erights erights added the automerge:rebase Automatically rebase updates, then merge label Apr 15, 2023
@mergify mergify bot merged commit 795c564 into master Apr 15, 2023
@mergify mergify bot deleted the markm-rep-label-like-instance-label branch April 15, 2023 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants