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

always init Endo in tests #8205

Merged
merged 6 commits into from
Aug 16, 2023
Merged

always init Endo in tests #8205

merged 6 commits into from
Aug 16, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Aug 16, 2023

refs: #2663

Description

Our test env setup has someproblems:

  1. The setup import has both side-effects and exports (the latter implying to tools that it doesn't have side-effects, resulting in moves and test breakage) (ses-ava vs install-ses import order matters when it should not. #2663, ses-ava should not mix exports and side-effects endojs/endo#1467)
  2. The side-effects are needed for all the tests in the package, but require each test to include them
  3. the ses-ava wrapper breaks features of Ava (Some ses-ava static test methods don't work endojs/endo#647)

This PR tackles #2, and progress towards the others. It configures some aspects of test env preparation to be package-scoped instead of file scoped. It does this by configuring the Ava runner to import the modules that have the desired side-effects. In this case, @endo/init/debug.js.

Many tests were using only that and then import ava from 'ava' instead of prepare-test-env-ava. This frees those tests of any side-effect imports.

With this pattern maybe we could also make a @endo/ses-ava/monkeypatch that modifies the the Ava module instead of wrapping. If we can do that in a way that's fully compatible with Ava then we could always run that patch and remove another test-level concern. Most tests at that point could just begin with import test from 'ava'.

The other setup tasks are adding reincarnate and/or VatData. I expect those are needed on a package basis and could have the same package-level config.

Security Considerations

--

Scaling Considerations

--

Documentation Considerations

Will require notifying developers when this lands that the old imports are redundant.

Testing Considerations

CI

Upgrade Considerations

--

@turadg turadg requested review from warner and gibson042 August 16, 2023 12:19
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Hooray for less ceremony in writing tests! Is there a package.json template somewhere that should be updated as well?

It also looks like this pulls the rug out from under some dapp testing ("ReferenceError: HandledPromise is not defined"), but I trust that can be addressed. At a glance, it seems like test-….js files in dapp-card-store contract/test/ and dapp-otc contract/test/ and documentation snippets/ expect an import of @agoric/zoe/tools/prepare-test-env.js (in the case of Agoric/documentation, via its own prepare-test-env-ava.js) to initialize a Hardened JS environment (and probably a vat-like one) via @agoric/zoe/tools/prepare-test-env.js importing @agoric/swingset-liveslots/tools/prepare-test-env.js, which this PR updates to no longer import @agoric/internal/src/install-ses-debug.js and thereby breaks the expectation.

@@ -1,5 +1,4 @@
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line import/no-extraneous-dependencies

@@ -1,6 +1,5 @@
/* eslint-disable ava/assertion-arguments -- the standard diff is unreadable */
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line import/no-extraneous-dependencies

@@ -1,5 +1,4 @@
// Must be first to set up globals
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Must be first to set up globals

@@ -1,5 +1,4 @@
// Must be first to set up globals
import '@endo/init/debug.js';
// Consider ses-ava once https://github.com/endojs/endo/issues/1235 is resolved
Copy link
Member

Choose a reason for hiding this comment

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

endojs/endo#1235 is resolved, but many test files are now fine with just import test from 'ava' so I think we can drop this comment.

Suggested change
// Consider ses-ava once https://github.com/endojs/endo/issues/1235 is resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants