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

🔧 adopt the naming convention for tests #38

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Conversation

qd-qd
Copy link
Member

@qd-qd qd-qd commented Aug 9, 2023

By adopting the convention, we blacklist non-relevant tests from the gas report. That produces a more realistic and accurate report. In addition, I decide to use the suffix _ReportSkip to blacklist tests that do not expect to revert but are not relevant to the gas report because they test a failure case. Helpful and relevant in the case of a library that returns a boolean that is supposed to return true when correctly used.

In this PR, gas reports are updated after we filtered the non-relevant tests.

@qd-qd qd-qd self-assigned this Aug 9, 2023
By adopting the convention, we blacklist non-relevant tests from the gas
report. That produces a more realistic and accurate report.
In addition, I decide to use the suffix `_ReportSkip` to blacklist tests
that do not expect to revert but are not relevant for the gas report
because they test a failure case. Helpful and relevant in a case of a
library that returns a boolean that is supposed to return true when
correctly used.
Now that we automatically filter non-relevant tests, numbers are much
more realistic.
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Changes to gas cost

Generated at commit: 2d7c5d06c8943e7dcdc3dafe9645b5ca909b6de8, compared to commit: 7d2f44ccea5d3d4e99fb66ae0308278f5da4b677

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
ImplementationECDSA256r1 verify +92,601 ❌ +83.91%
ImplementationECDSA256r1Precompute verify +30,471 ❌ +67.85%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
ImplementationECDSA256r1 1,002,641 (0) mulmuladd
verify
27,533 (+26,923)
192,620 (+192,083)
+4413.61%
+35769.65%
27,611 (+9,001)
202,959 (+92,601)
+48.37%
+83.91%
27,611 (+78)
202,905 (+5,433)
+0.28%
+2.75%
27,689 (0)
210,079 (-2,110)
0.00%
-0.99%
2 (-1)
57 (-68)
ImplementationECDSA256r1Precompute 675,908 (0) verify 74,589 (+74,002) +12606.81% 75,378 (+30,471) +67.85% 75,507 (+16,211) +27.34% 75,507 (0) 0.00% 57 (-85)

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

LCOV of commit 7b045f3 during Tests #174

Summary coverage rate:
  lines......: 90.3% (84 of 93 lines)
  functions..: 100.0% (9 of 9 functions)
  branches...: no data found

Files changed coverage rate: n/a

@qd-qd
Copy link
Member Author

qd-qd commented Aug 9, 2023

⚠️ Do not trust the gas diff report in this PR ⚠️

Gas consumption didn't change, we only blacklisted non-relevant tests that reverted. As those tests consumed less gas than the successful ones (as the execution is stopped before the end of the computation), the average consumption seems to raise. The logic didn't change, but the previous gas snapshots were misleading. This PR fixes that.

@qd-qd qd-qd merged commit 7b045f3 into main Aug 9, 2023
4 checks passed
@qd-qd qd-qd deleted the improve-gas-report branch August 9, 2023 15:35
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.

1 participant