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.

Sign up for GitHub

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

Jump to bottom

Sync Endo 2023-11 #8514

Closed
wants to merge 11 commits into from
Closed

Sync Endo 2023-11 #8514

wants to merge 11 commits into from

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented Nov 9, 2023 •
edited
Loading

No description provided.

@kriskowal kriskowal force-pushed the kriskowal-sync-endo-2023-11 branch 2 times, most recently from b212093 to bb068f6 Compare November 28, 2023 01:08
@kriskowal kriskowal force-pushed the kriskowal-sync-endo-2023-11 branch 7 times, most recently from 2d73d09 to b693ab6 Compare December 7, 2023 01:57
@mergify mergify bot mentioned this pull request Dec 7, 2023
Closed
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 7, 2023

⚠️ The sha of the head commit of this PR conflicts with #8632. Mergify cannot evaluate rules on this PR. ⚠️

@kriskowal kriskowal force-pushed the kriskowal-sync-endo-2023-11 branch 7 times, most recently from 69801d4 to 21a95f0 Compare December 9, 2023 00:28
@kriskowal kriskowal mentioned this pull request Dec 9, 2023
Merged
erights
erights approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Approving FWIW. You should probably still wait for another reviewer.

packages/swingset-xsnap-supervisor/scripts/build-bundle.js Outdated Show resolved Hide resolved
packages/xsnap-lockdown/scripts/build-bundle.js Outdated Show resolved Hide resolved
packages/zoe/test/unitTests/test-zoe-startInstance.js
@@ -23,24 +23,35 @@ test('bad installation', async t => {

function isEmptyFacet(t, facet) {
t.is(passStyleOf(facet), 'remotable');
t.deepEqual(Object.getOwnPropertyNames(facet), []);
t.deepEqual(
Object.getOwnPropertyNames(facet).filter(name => !name.startsWith('__')),
Copy link
Member

Choose a reason for hiding this comment

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

Factor out reusable testing helper?

packages/zoe/test/unitTests/test-zoe.js Outdated
@@ -134,7 +134,9 @@ test(`E(zoe).getPublicFacet - no instance`, async t => {
// @ts-expect-error deliberate invalid arguments for testing
await t.throwsAsync(() => E(zoe).getPublicFacet(), {
message:
/In "getPublicFacet" method of \(ZoeService\): arg 0: .*"\[undefined\]" - Must be a remotable/,
// Or pattern of golden error tolerant across versions of endo.
Copy link
Member

Choose a reason for hiding this comment

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

I know I wrote this horrible sentence, but how about

Suggested change
// Or pattern of golden error tolerant across versions of endo.
// Golden test uses RegExp "Or" pattern to tolerate earlier versions of endo

Likewise below

packages/zoe/test/unitTests/zcf/test-zcf.js
@@ -707,7 +707,10 @@ test(`zcfSeat from zcf.makeEmptySeatKit - only these properties exist`, async t
const { zcf } = await setupZCFTest();
const makeZCFSeat = () => zcf.makeEmptySeatKit().zcfSeat;
const seat = makeZCFSeat();
t.deepEqual(getStringMethodNames(seat), expectedStringMethods.sort());
t.deepEqual(
getStringMethodNames(seat).filter(name => !name.startsWith('__')),
Copy link
Member

Choose a reason for hiding this comment

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

If you create reusable testing helper for these, see if you can use it here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s my preference to keep tests readable at the expense of repetition. In this case, I believe the code is offering an explanation.

packages/zone/src/durable.js
@@ -89,6 +89,10 @@ export const makeDurableZone = (baggage, baseLabel = 'durableZone') => {
/** @type {import('.').Zone['exoClass']} */
const exoClass = (...args) => prepareExoClass(baggage, ...args);
/** @type {import('.').Zone['exoClassKit']} */
// @ts-ignore This type check regressed inexplicably with the release
Copy link
Member

Choose a reason for hiding this comment

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

I see lint gave you the expected second order error. I have suppressed this either with another @ts-ignore or an eslint-disable-next-line . I forget which. In any case, I suspect plenty of these remain in the code.

This was referenced Dec 11, 2023
Merged
Closed
Merged
@turadg turadg mentioned this pull request Dec 11, 2023
Merged
@kriskowal kriskowal force-pushed the kriskowal-sync-endo-2023-11 branch 2 times, most recently from 763092b to 98e6c2b Compare December 11, 2023 23:14
@kriskowal kriskowal mentioned this pull request Dec 12, 2023
Merged
@kriskowal
Copy link
Member Author

Subsumed this into #8651 which goes on to sync up with published Endo versions.

@kriskowal kriskowal closed this Dec 12, 2023
@erights
Copy link
Member

erights commented Dec 12, 2023

I can also close #8642 as subsumed, yes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhofman mhofman mhofman left review comments

@erights erights erights approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kriskowal @erights @mhofman