test: snapshot of runtime exports #10202
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with
|
| Latest commit: |
636b357
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8adbdfe0.agoric-sdk.pages.dev |
| Branch Preview URL: | https://10086-regression.agoric-sdk.pages.dev |
mhofman
approved these changes
Oct 3, 2024
There was a problem hiding this comment.
Oh this is neat!
One small observation: this doesn't allow us to detect default exports, but I believe none of our packages would ever do that. Dynamic import would allow us to detect this (while also enabling us to handle more explicitly import issues), but it would open us to thenable module risks, so I'm split.
I wish we could do the same for type exports. Maybe with the help of something like https://gist.github.com/Glavin001/6281f12ee97f40fb8fbde5a319457119 ?
turadg
force-pushed
the
10086-regression
branch
2 times, most recently
from
a65f0cb to
95e0b4a
Compare
ava-xs lacks t.snapshot
turadg
force-pushed
the
10086-regression
branch
from
95e0b4a to
636b357
Compare
mergify bot
added a commit
that referenced
this pull request
Oct 4, 2024
incidental ## Description While working on #10202 I got a CI error that "no function" for the Zoe tests. It turned out to be that ava-xs doesn't support `t.snapshot`. So I added it to the voluminous "exclude" value and pushed again. Later I see CI [fails again](https://github.com/Agoric/agoric-sdk/actions/runs/11162498228/job/31027478876?pr=10202) because ava-xs is confused by the `snapshots` directory. The `test:xs-unit` job is not carrying its weight. I can't recall it ever detecting an error that wasn't detected in Node. Maybe a while ago it did and it was an important line of defense. In the past year we've developed much more integration testing and I'm confident these tests are redundant. It generally doesn't hurt to test more, but ava-xs is so different from XS that the cost of reconciliation is too great. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations see above ### Upgrade Considerations none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #10086
Description
#10086 has been fixed by other PRs but still needed a regression test showing that runtime importing from
@agoric/orchestrationdoes not fail.This goes a step further and snapshots the list of exports, so that any changes are seen in code review.
It also does it for a number of other active packages that tend to be imported outside the repo.
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
It's a fast test. I also considered making a test in
bootthat iterated over the whole workspace, but I thought this would be an easier workflow and allow for more local tailoring.Upgrade Considerations
none