Make bench-canister use a better Bench type #371
Merged
Conversation
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
The bench-canister previously expected a type for the Bench passed in by the user that was too narrow. The expected type included public setter functions in Bench. Therefore it was making an assumption about implementation details of Bench on the user's side. That was unnecessary and restrictive. This commit changes the expected to a suitable supertype which only expects the getter functions that are actually used by the bench-canister. Plus it also expects the currently still unused getVersion getter, but which is likely to be used in the future. bench-canister no longer imports the `bench` package. This makes sense because the purpose the `bench` package is as a helper to the user to write Benches. The only duplicated code between bench-canister and the `bench` package is the BenchSchema type. Still, this duplication makes sense because bench-canister defines what it needs and the package re-implements the type on the user side for convenience to the user.
rvanasa
previously approved these changes
Jan 26, 2026
Contributor
rvanasa
left a comment
rvanasa
left a comment
There was a problem hiding this comment.
LGTM! Thank you for this simplification of how benchmarking works for Mops.
Contributor
Author
|
Please approve workflows again |
rvanasa
previously approved these changes
Jan 26, 2026
Contributor
Author
|
Next try, please approve workflows again. I don't know what goes wrong in "Run benchmark and comment" in step "Create or Update Comment" though. That might still not work. |
Contributor
|
Thanks for looking into it. I'm just going to merge expecting that it's related to the PR originating from a fork. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The bench-canister previously expected a type for the Bench passed in by the user that was too narrow. The expected type included public setter functions in Bench even though the bench-canister does not call setter functions. Therefore it was making an assumption about implementation details of Bench on the user's side. That was unnecessary and restrictive.
This commit changes the expected type to a suitable supertype which only includes the getter functions that are actually used by the bench-canister. Plus, it also expects the currently still unused getVersion getter because that one is likely to be used in the future.
bench-canister no longer imports the
benchpackage. This makes sense because the purpose of thebenchpackage is as a helper to the user to write Benches. It chooses one specific implementation of Benches, something that is irrelevant to the bench-canister. The only duplicated code between bench-canister and thebenchpackage is theBenchSchematype. Still, this duplication makes sense because bench-canister defines what it needs and the package re-implements the type on the user side for convenience to the user.