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.

Sign up for GitHub

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

Jump to bottom

ci: fix matrix exclude for solidity packages #3255

Merged
merged 3 commits into from
Oct 9, 2024
Merged

ci: fix matrix exclude for solidity packages #3255

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b869aa
ci: fix exclude keyword
ChiTimesChi Oct 9, 2024
7c298ac
ci: try adding no-op benchmark tests
ChiTimesChi Oct 9, 2024
3cf20c3
Revert "ci: fix exclude keyword"
ChiTimesChi Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions .github/workflows/solidity.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ jobs:
fail-fast: false
matrix:
package: ${{ fromJson(needs.changes.outputs.packages) }}
# Slither is irrelevant for solidity-devops, as it only contains devops scripts rather than deployed contracts
exclude:
- package: solidity-devops
# Slither is irrelevant for solidity-devops, as it only contains devops scripts rather than deployed contracts
exclude:
- package: solidity-devops
permissions:
# always required
security-events: write
Expand Down Expand Up @@ -246,8 +246,8 @@ jobs:
matrix:
package: ${{ fromJson(needs.changes.outputs.packages) }}
# Gas diff is irrelevant for solidity-devops, as it only contains devops scripts rather than deployed contracts
exclude:
- package: solidity-devops
exclude:
- package: solidity-devops
steps:
- uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -296,9 +296,9 @@ jobs:
fail-fast: false
matrix:
package: ${{ fromJson(needs.changes.outputs.packages) }}
# Size check is irrelevant for solidity-devops, as it only contains devops scripts rather than deployed contracts
exclude:
- package: solidity-devops
# Size check is irrelevant for solidity-devops, as it only contains devops scripts rather than deployed contracts
exclude:
- package: solidity-devops
steps:
- uses: actions/checkout@v4
with:
Expand Down
10 changes: 10 additions & 0 deletions packages/contracts-core/test/GasBenchmark.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Test} from "forge-std/Test.sol";

/// @notice No gas benchmark is required for contracts-core
contract GasBenchmarkTest is Test {
Comment on lines +6 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

// solhint-disable-next-line no-empty-blocks
function testGasBenchmark() public {}
Comment on lines +8 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. If gas benchmarking is needed, implement the actual benchmark logic.
  2. If no benchmark is required, remove this function and potentially the entire contract.
  3. 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.

Suggested change
// 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
}

}
10 changes: 10 additions & 0 deletions packages/solidity-devops/test/GasBenchmark.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Test} from "forge-std/Test.sol";

/// @notice No gas benchmark is required for solidity-devops
contract GasBenchmarkTest is Test {
// solhint-disable-next-line no-empty-blocks
function testGasBenchmark() public {}
}
Loading