-
Notifications
You must be signed in to change notification settings - Fork 2
Simplify the certified-transaction wire format, and test #146 against a real aggregator #147
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
242d7a1
Address review findings on the service-time change
MastaP 3fb2a4b
Correct what the round-timestamp bound actually guarantees
MastaP 4b40222
Call the local-stack suite integration, and update the aggregator image
MastaP 683d32c
Let Testcontainers own the aggregator stack, and run it in CI
MastaP 50e0c16
Gate npm publishing on the integration job
MastaP 4d90a78
Release as 3.0.0 and document the break
MastaP 0c0b00b
State the units on expiresAt and reference time everywhere
MastaP 074e95d
Drop the shell-managed path; Testcontainers owns the stack outright
MastaP File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import base from './jest.config.js'; | ||
|
|
||
| /** | ||
| * Integration suite: the same transforms as the default config, plus the | ||
| * aggregator stack the tests run against. | ||
| * | ||
| * It lives in its own config because globalSetup is per-run, and starting an | ||
| * aggregator for the unit and functional suites — which have no service to talk | ||
| * to — would put a docker dependency on the tests that are meant not to have | ||
| * one. Coverage is off: these exercise wire compatibility, and the unit and | ||
| * functional suites are what measure reach into src/. | ||
| */ | ||
| export default { | ||
| ...base, | ||
| collectCoverage: false, | ||
| globalSetup: '<rootDir>/tests/integration/support/globalSetup.mjs', | ||
| globalTeardown: '<rootDir>/tests/integration/support/globalTeardown.mjs', | ||
| testMatch: ['<rootDir>/tests/integration/**/*Test.ts'], | ||
| }; |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the real-aggregator integration suite fails but the unit/build job succeeds, this sibling job does not prevent the
buildjob's main-branch or release steps from publishing to npm; GitHub Actions runs jobs withoutneedsindependently, so an incompatible package can be published before the workflow ultimately turns red. Make publication depend on successful completion of this integration job, or move the publish steps into a downstream job.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in 50e0c16. You're right, and this one is on me for making it worse: before this PR there was no integration job at all, so adding one that looks like a gate without being one is arguably worse than not having it — a reviewer reads the workflow and assumes releases are covered.
Publishing now lives in its own job:
needsis the gate; the event conditions only choose which kind of release it is. I took the second of your two options — a downstream job rather thanneeds: integrationonbuild— so that PR feedback stays parallel: makingbuildwait onintegrationwould serialise every PR for a constraint that only matters on main and on release.Two consequences of the split worth noting for review:
id-token: writeis only needed to publish, sobuildandintegrationno longer request it.npm ciandnpm run build, since jobs don't share a filesystem andfilesin package.json shipslib/. That's ~45s of duplicated work, which seemed the right trade against publishing an unbuilt package.One thing I checked while making the change: I had initially renamed the
buildjob to "Build and test", which would have changed the reported check name. There is no branch protection on this repo today, so nothing would have broken — but a future required check configured asbuildwould have silently never matched. Reverted; the job keeps its bare name, and only the job body changed.