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

Add gas tests for sierra-gas tracked resource #3055

Open
wants to merge 1 commit into
base: szymczyk/sg-6-new-gas-representation
Choose a base branch
from

Conversation

THenry14
Copy link
Contributor

@THenry14 THenry14 commented Mar 6, 2025

Related to #2977

This PR stack aims to add support for new resource tracking (sierra-gas) and new gas representation (GasVector triplet instead of single l1 gas value).

-- (#3049) Add sierra gas tracking
-- (#3050) Run already existing tests with CairoSteps set explicitly
-- (#3051) Add support for sierra gas in --detailed-resources
-- (#3052) Add support for sierra gas in --save-trace-data
-- (#3053) Add sierra version assertion
-- (#3054) Introduce new gas representation
--> (This PR) Add gas tests for sierra-gas tracking
-- (#3056) Update docs with sierra-gas related changes

@THenry14 THenry14 force-pushed the szymczyk/sg-6-new-gas-representation branch from a0e19c5 to a50a267 Compare March 10, 2025 12:36
@THenry14 THenry14 force-pushed the szymczyk/sg-7-tests-for-sierra-based-gas branch from 9ced0b4 to 31bb10a Compare March 10, 2025 12:36
@THenry14 THenry14 force-pushed the szymczyk/sg-6-new-gas-representation branch from a50a267 to f134013 Compare March 10, 2025 12:52
Copy link
Contributor

@ddoktorski ddoktorski left a comment

Choose a reason for hiding this comment

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

Even with Scarb 2.11.1 some of these tests already fail. Based on our experience so far, such tests seem unmaintainable in the long run. I don't have a specific idea on how to test gas related features, but could we at least assert all values with some acceptable margin? 🙏

cc @cptartur

@THenry14 THenry14 force-pushed the szymczyk/sg-7-tests-for-sierra-based-gas branch from 31bb10a to cbeeaec Compare March 10, 2025 12:53
@THenry14
Copy link
Contributor Author

Even with Scarb 2.11.1 some of these tests already fail. Based on our experience so far, such tests seem unmaintainable in the long run. I don't have a specific idea on how to test gas related features, but could we at least assert all values with some acceptable margin? 🙏

cc @cptartur

You mean that tests fail when you bump scarb? I guess this is something that can be expected right?

but could we at least assert all values with some acceptable margin?

Seems like a brand new issue tbh, not sure this is something that should be introduced here 🤔 It would probably require us to hold a matrix with prices per version? or even worse, writing some heuristic to "guess" the values? It is probably hard to predict how the cost can change.

As I said, I think this is good idea, just not in scope of this issue.

@THenry14 THenry14 requested a review from ddoktorski March 10, 2025 13:03
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.

3 participants