ci: fix matrix exclude for solidity packages #3255
Conversation
github-actions
bot
added
M-ci
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This reverts commit 4b869aa.
Deploying sanguine-fe with
|
| Latest commit: |
7c298ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://be1a9e4b.sanguine-fe.pages.dev |
| Branch Preview URL: | https://ci-matrix-package-exclude.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/solidity-devops/test/GasBenchmark.t.sol (1)
1-2: Consider upgrading Solidity versionThe current Solidity version (^0.8.0) is relatively recent, but you might want to consider upgrading to a more recent version (e.g., ^0.8.19 or ^0.8.20) to take advantage of the latest optimizations and security improvements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/solidity.yml (3 hunks)
- packages/contracts-core/test/GasBenchmark.t.sol (1 hunks)
- packages/solidity-devops/test/GasBenchmark.t.sol (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/solidity.yml
133-133: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
249-249: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
300-300: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
🔇 Additional comments (7)
packages/solidity-devops/test/GasBenchmark.t.sol (3)
4-4: LGTM: Appropriate import for Forge testThe import statement is correct and follows best practices by importing only the necessary
Testcontract from Forge's standard library.
6-7: Clarify the purpose of this contractThe comment states that no gas benchmark is required for solidity-devops, yet this contract is named
GasBenchmarkTest. If no benchmark is needed, consider explaining why this contract exists or if it should be removed to avoid confusion.Could you provide more context on why this contract is necessary if no gas benchmark is required?
8-9:⚠️ Potential issueClarify the purpose of the empty
testGasBenchmarkfunctionThe
testGasBenchmarkfunction is currently empty, which seems to align with the comment stating no gas benchmark is required. However, this raises a few questions:
- If no benchmark is needed, why does this function exist?
- If a benchmark is intended in the future, should there be a TODO comment explaining what needs to be implemented?
- Could this function be removed if it's not serving any purpose?
Consider either removing the function if it's not needed, or adding a comment explaining its future use if it's a placeholder.
Could you provide more information on the intended use of this function?
.github/workflows/solidity.yml (4)
132-134: LGTM: Appropriate exclusion for solidity-devops in the slither jobThe exclusion of the
solidity-devopspackage from theslitherjob is correctly implemented and well-commented. This is appropriate since the package contains only devops scripts rather than deployed contracts, making Slither analysis unnecessary.Note: The static analysis tool flagged the
excludekey as unexpected, but this is a false positive. Theexcludekey is valid and commonly used in GitHub Actions workflow files under thestrategysection to exclude specific matrix combinations.🧰 Tools
🪛 actionlint
133-133: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
249-250: LGTM: Appropriate exclusion for solidity-devops in the gas-diff jobThe exclusion of the
solidity-devopspackage from thegas-diffjob is correctly implemented and well-commented. This exclusion is appropriate since the package contains only devops scripts rather than deployed contracts, making gas diff analysis irrelevant.Note: As with the previous section, the static analysis tool's warning about the
excludekey can be safely ignored as it's a false positive.🧰 Tools
🪛 actionlint
249-249: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
299-301: LGTM: Appropriate exclusion for solidity-devops in the size-check jobThe exclusion of the
solidity-devopspackage from thesize-checkjob is correctly implemented and well-commented. This exclusion is appropriate since the package contains only devops scripts rather than deployed contracts, making size check analysis irrelevant.As with the previous sections, the static analysis tool's warning about the
excludekey can be safely ignored as it's a false positive in the context of GitHub Actions workflows.🧰 Tools
🪛 actionlint
300-300: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
Line range hint
132-301: Summary: Improved workflow efficiency by excluding solidity-devops from irrelevant jobsThe changes made to this workflow file consistently exclude the
solidity-devopspackage from theslither,gas-diff, andsize-checkjobs. These exclusions are appropriate and well-implemented for the following reasons:
- They prevent unnecessary processing of a package that contains only devops scripts and no deployed contracts.
- Each exclusion is accompanied by a clear comment explaining the rationale, which enhances maintainability.
- The exclusions are correctly implemented using the
excludekey under thestrategysection in each relevant job.These changes will improve the efficiency of the CI pipeline by avoiding irrelevant analyses on the
solidity-devopspackage. The added comments also make the workflow more understandable for future maintainers.Note: The static analysis warnings about the
excludekey can be safely ignored, as they are false positives in the context of GitHub Actions workflows.🧰 Tools
🪛 actionlint
249-249: unexpected key "exclude" for "strategy" section. expected one of "fail-fast", "matrix", "max-parallel"
(syntax-check)
| // solhint-disable-next-line no-empty-blocks | ||
| function testGasBenchmark() public {} |
There was a problem hiding this comment.
Reconsider the empty test function
The testGasBenchmark() function is currently empty. Empty test functions don't provide any value and may give a false sense of security.
Consider one of the following options:
- If gas benchmarking is needed, implement the actual benchmark logic.
- If no benchmark is required, remove this function and potentially the entire contract.
- If this is a placeholder for future implementation, add a TODO comment explaining the intended functionality.
Example of option 3:
// solhint-disable-next-line no-empty-blocks
-function testGasBenchmark() public {}
+function testGasBenchmark() public {
+ // TODO: Implement gas benchmark logic for specific core contract operations
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // solhint-disable-next-line no-empty-blocks | |
| function testGasBenchmark() public {} | |
| // solhint-disable-next-line no-empty-blocks | |
| function testGasBenchmark() public { | |
| // TODO: Implement gas benchmark logic for specific core contract operations | |
| } |
| /// @notice No gas benchmark is required for contracts-core | ||
| contract GasBenchmarkTest is Test { |
There was a problem hiding this comment.
💡 Codebase verification
Remove the unnecessary GasBenchmarkTest contract
The comment indicates that no gas benchmark is required for contracts-core, yet the GasBenchmarkTest contract is defined in packages/contracts-core/test/GasBenchmark.t.sol without any references or functionality. Consider removing this contract to eliminate confusion and maintain consistency.
🔗 Analysis chain
Clarify the purpose of this contract
The comment states that no gas benchmark is required for contracts-core, yet a GasBenchmarkTest contract is defined. This seems contradictory and may lead to confusion.
Could you please clarify the intended purpose of this contract? If no gas benchmark is truly required, consider removing this file entirely.
Retrying search with correct file extension for Solidity files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to GasBenchmarkTest in the codebase
# Test: Search for references to GasBenchmarkTest
rg --type solidity "GasBenchmarkTest"
Length of output: 70
Script:
#!/bin/bash
# Description: Check for references to GasBenchmarkTest in Solidity files
rg -g "*.sol" "GasBenchmarkTest"
Length of output: 621
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3255 +/- ##
============================================
Coverage ? 93.10658%
============================================
Files ? 89
Lines ? 2205
Branches ? 278
============================================
Hits ? 2053
Misses ? 143
Partials ? 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
GasBenchmarkTestcontracts in bothcontracts-coreandsolidity-devopspackages for future gas benchmarking tests.solidity-devopspackage from specific jobs in the workflow configuration.