test coverage over bundles by turadg · Pull Request #11100 · Agoric/agoric-sdk

turadg · 2025-03-10T21:55:30Z

closes: #1817

Description

Code coverage for bundles in unit tests!

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

per se

Upgrade Considerations

none

cloudflare-workers-and-pages · 2025-03-10T21:55:31Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ea9dcce`
Status:	✅ Deploy successful!
Preview URL:	https://fe9fd11d.agoric-sdk.pages.dev
Branch Preview URL:	https://1817-bundle-coverage.agoric-sdk.pages.dev

View logs

mhofman · 2025-03-11T01:27:25Z

+        moduleFormat: 'test',
+        [Symbol.for('exports')]: exports,
+      });
+      id = `b1-test-exports-${Math.random()}`;


Interesting, this will also ensure that bundleAndInstall is only used from the privileged start compartment.

mhofman · 2025-03-11T23:12:33Z

Draft until this doesn't have to patch Endo (next Endo rev )

I'd like to emphasize this one. The next Endo version introduces a revamped bundler, and maintaining coverage through the endo sync will be invaluable for @kriskowal

mhofman · 2025-03-11T23:19:18Z

+      // Copy all the properties so this object can be hardened.
+      const exports = { ...pathOrExports };
+      return bundleTestExports(exports);


Does anyone harden the returned bundle?

not presently.

https://github.com/endojs/endo/blob/84716b7fb874f790000b96953ef6fe194c4a2f85/packages/import-bundle/src/index.js#L213-L223

If you think that's necessary I'd request that it be done in ^ so the current example code suffices:
https://github.com/endojs/endo/blob/84716b7fb874f790000b96953ef6fe194c4a2f85/packages/import-bundle/README.md?plain=1#L81-L82

I think my question is can we get away with dropping the spread if no code would harden the bundle.

If there is such code, then my question for @kriskowal is whether bundleTestExports should create a getter for the Symbol.for('exports') property instead of a data property so that it's compatible with hardening? That should be a compatible change. It will not make the bundle passable but that is just not possible with this design.

Given that this doesn't hurt, and is probably helpful (no-one should expect a live binding here), I'm ok with keeping the spread here.

kriskowal

@turadg It’s fantastic we can get coverage analysis for contracts this way. Do you feel that, after this change, we still have an adequate canary for changes the contract that will only be evident if it’s bundled, like runtime censorship errors?

I’ve filed a work item for Endo, to make the bundler do a SES censorship simulation after transforms, such that the act of bundling in CI would detect that failure mode without having to execute the bundle.
endojs/endo#2756

That might be enough.

turadg · 2025-04-03T21:05:07Z

Do you feel that, after this change, we still have an adequate canary for changes the contract that will only be evident if it’s bundled, like runtime censorship errors?

Yes, this is just letting us integrate with a fake Zoe and we have a more integration levels above this.

Boostrap/RunUtils with a testing SwingSet, but all the vats and real bundles
a3p-integration with a real agoric chain

mhofman · 2025-04-04T00:38:40Z

Do you feel that, after this change, we still have an adequate canary for changes the contract that will only be evident if it’s bundled, like runtime censorship errors?

Yes, this is just letting us integrate with a fake Zoe and we have a more integration levels above this.

Catching early would still be better, so endojs/endo#2756 would be a nice to have

mhofman · 2025-04-04T01:26:13Z

+      // Copy all the properties so this object can be hardened.
+      const exports = { ...pathOrExports };
+      return bundleTestExports(exports);


I think my question is can we get away with dropping the spread if no code would harden the bundle.

If there is such code, then my question for @kriskowal is whether bundleTestExports should create a getter for the Symbol.for('exports') property instead of a data property so that it's compatible with hardening? That should be a compatible change. It will not make the bundle passable but that is just not possible with this design.

mhofman

Thanks for getting us this coverage!

mhofman · 2025-04-04T18:05:39Z

+      // Copy all the properties so this object can be hardened.
+      const exports = { ...pathOrExports };
+      return bundleTestExports(exports);


Given that this doesn't hurt, and is probably helpful (no-one should expect a live binding here), I'm ok with keeping the spread here.

mergify · 2025-04-04T18:11:42Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Chris-Hibbert · 2025-04-04T18:19:23Z

 import { commonSetup } from '../supports.js';

-const dirname = path.dirname(new URL(import.meta.url).pathname);
+import * as contractExports from '../../src/examples/stake-ica.contract.js';


Why doesn't import path from 'path'; get dropped here and in most, if not all of these files?

Because unused imports aren't automatically cleaned up. It's passing CI because there's no lint rule for it.

I would expect no-unused-vars to but it's not. typescript-eslint/no-unused-vars works but isn't worth the effort to adopt rn, imo. Maybe updating eslint will fix the built in one.

noUsedLocals in tsconfig would catch it but make it harder to work with jsdoc @import.

turadg force-pushed the 1817-bundle-coverage branch from 608f963 to 6f954ac Compare March 10, 2025 23:58

turadg requested review from Chris-Hibbert and kriskowal March 11, 2025 00:00

Chris-Hibbert reviewed Mar 11, 2025

View reviewed changes

Comment thread packages/zoe/tools/setup-zoe.js Outdated

mhofman reviewed Mar 11, 2025

View reviewed changes

turadg force-pushed the 1817-bundle-coverage branch 2 times, most recently from addd80a to 64eb491 Compare April 2, 2025 23:02

turadg marked this pull request as ready for review April 3, 2025 14:39

turadg requested a review from a team as a code owner April 3, 2025 14:39

turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 3, 2025

turadg force-pushed the 1817-bundle-coverage branch from 64eb491 to 65c6cf4 Compare April 3, 2025 17:47

turadg requested review from Chris-Hibbert and mhofman April 3, 2025 17:47

turadg force-pushed the 1817-bundle-coverage branch from 65c6cf4 to 6487f51 Compare April 3, 2025 17:51

kriskowal reviewed Apr 3, 2025

View reviewed changes

turadg requested a review from kriskowal April 3, 2025 22:15

turadg marked this pull request as draft April 4, 2025 00:13

mhofman reviewed Apr 4, 2025

View reviewed changes

turadg added 5 commits April 4, 2025 09:35

feat: fakeVatAdmin allow unserializable bundles

59527fc

feat: setUpZoeForTest supports 'test' bundles

994102a

test: code coverage for contract test

7072b3f

test(psm): code coverage

267b5d2

test: bootstrapPayment coverage

ea9dcce

turadg force-pushed the 1817-bundle-coverage branch from 6487f51 to ea9dcce Compare April 4, 2025 16:37

turadg marked this pull request as ready for review April 4, 2025 17:18

turadg requested a review from mhofman April 4, 2025 17:18

mhofman approved these changes Apr 4, 2025

View reviewed changes

Chris-Hibbert approved these changes Apr 4, 2025

View reviewed changes

mergify Bot merged commit 3190128 into master Apr 4, 2025

mergify Bot deleted the 1817-bundle-coverage branch April 4, 2025 18:48

Conversation

turadg commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Uh oh!

cloudflare-workers-and-pages Bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying agoric-sdk with Cloudflare Pages

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

turadg commented Apr 3, 2025

Uh oh!

mhofman commented Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

turadg commented Mar 10, 2025 •

edited

Loading

cloudflare-workers-and-pages Bot commented Mar 10, 2025 •

edited

Loading