Improve slither-mutate testing #2482
Conversation
DarkaMaul
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughWalkthroughThe recent updates introduce improvements and new features to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestRunner
participant Mutator
participant FileHandler
participant SolidityContract
participant Foundry
User->>TestRunner: Run tests
TestRunner->>Mutator: Execute main functionality
Mutator->>FileHandler: Backup source files
FileHandler->>Mutator: Return backup status
Mutator->>SolidityContract: Process Solidity files
SolidityContract->>Mutator: Return processing results
Mutator->>TestRunner: Return test results
TestRunner->>User: Display test results
User->>Foundry: Configure settings
Foundry-->>User: Confirm configuration
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
slither/tools/mutator/utils/file_handling.py (1)
Line range hint
33-33: Remove unnecessary open mode parameters.- with (mutation_dir / mutation_filename).open("w", encoding="utf8") as mutant_file: + with (mutation_dir / mutation_filename).open(encoding="utf8") as mutant_file:This change aligns with the static analysis suggestion to remove unnecessary parameters and improve code clarity.
Also applies to: 88-88
slither/tools/mutator/utils/testing_generated_mutant.py (2)
Line range hint
21-21: Avoid using bareexcept.Replace the bare
exceptwith a more specific exception to avoid catching unexpected errors.- except: + except Exception as e:
Line range hint
48-55: Usecapture_outputinstead ofPIPE.- stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + capture_output=True,This change simplifies the subprocess call and is in line with best practices.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- slither/tools/mutator/mutators/abstract_mutator.py (4 hunks)
- slither/tools/mutator/utils/file_handling.py (1 hunks)
- slither/tools/mutator/utils/testing_generated_mutant.py (1 hunks)
- tests/tools/mutator/test_data/test_source_unit/README.md (1 hunks)
- tests/tools/mutator/test_data/test_source_unit/foundry.toml (1 hunks)
- tests/tools/mutator/test_data/test_source_unit/script/Counter.s.sol (1 hunks)
- tests/tools/mutator/test_data/test_source_unit/src/Counter.sol (1 hunks)
- tests/tools/mutator/test_data/test_source_unit/test/Counter.t.sol (1 hunks)
- tests/tools/mutator/test_mutator.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/tools/mutator/test_data/test_source_unit/foundry.toml
Additional context used
Markdownlint
tests/tools/mutator/test_data/test_source_unit/README.md
21-21: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
27-27: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
33-33: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
39-39: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
45-45: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
51-51: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
57-57: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
63-63: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
64-64: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
65-65: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
14-14: null (MD034, no-bare-urls)
Bare URL used
Ruff
slither/tools/mutator/utils/file_handling.py
33-33: Unnecessary open mode parameters (UP015)
Remove open mode parameters
88-88: Unnecessary open mode parameters (UP015)
Remove open mode parameters
slither/tools/mutator/utils/testing_generated_mutant.py
21-21: Do not use bare
except(E722)
48-55: Prefer
capture_outputover sendingstdoutandstderrtoPIPE(UP022)Replace with
capture_outputkeyword argument
100-100: Unnecessary open mode parameters (UP015)
Remove open mode parameters
Additional comments not posted (12)
tests/tools/mutator/test_data/test_source_unit/script/Counter.s.sol (2)
7-7: ThesetUpfunction is currently empty. Ensure that this is intentional and consider documenting its intended use or future extensions.
9-11: Therunfunction correctly usesvm.broadcast()for transaction broadcasting. Ensure that any transaction-specific logic that needs to be tested is included here in future updates.tests/tools/mutator/test_data/test_source_unit/src/Counter.sol (2)
7-8: ThesetNumberfunction correctly updates the state variablenumber. Ensure that access controls are considered if needed in future iterations.
11-12: Theincrementfunction correctly increments the state variablenumber. Consider adding overflow checks if the contract will handle large numbers or critical operations.tests/tools/mutator/test_data/test_source_unit/test/Counter.t.sol (3)
10-12: ThesetUpfunction correctly initializes theCountercontract and sets the initial state. This is crucial for ensuring consistent test conditions.
15-17: Thetest_Incrementfunction correctly tests the increment functionality of theCountercontract. Good use of assertions to validate the expected state.
20-22: ThetestFuzz_SetNumberfunction effectively tests thesetNumberfunctionality with fuzzed inputs. This is a good practice for ensuring robustness against a wide range of inputs.slither/tools/mutator/utils/file_handling.py (1)
114-114: Ensure only files are processed inget_sol_file_list.This change correctly implements the PR's objective to avoid directories being mistaken for files.
tests/tools/mutator/test_mutator.py (2)
59-59: Ensure slow tests are skipped.The use of
@pytest.mark.skipis appropriate here to avoid running slow tests automatically.
42-57: Mocking strategy forargparse.Proper use of
mock.patchto simulate command line arguments for testing. This ensures that the function can be tested in isolation.slither/tools/mutator/mutators/abstract_mutator.py (2)
30-30: Use ofUniontype hint forsolc_remappings.This change correctly updates the type hint to be more explicit, which improves code readability and type safety.
84-84: Ensure correct handling of mutation results.The logic to handle mutation results and update counts is correctly implemented. This ensures accurate tracking of mutation outcomes.
Also applies to: 149-149
| ## Foundry | ||
|
|
||
| **Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.** | ||
|
|
||
| Foundry consists of: | ||
|
|
||
| - **Forge**: Ethereum testing framework (like Truffle, Hardhat and DappTools). | ||
| - **Cast**: Swiss army knife for interacting with EVM smart contracts, sending transactions and getting chain data. | ||
| - **Anvil**: Local Ethereum node, akin to Ganache, Hardhat Network. | ||
| - **Chisel**: Fast, utilitarian, and verbose solidity REPL. | ||
|
|
||
| ## Documentation | ||
|
|
||
| https://book.getfoundry.sh/ | ||
|
|
||
| ## Usage | ||
|
|
||
| ### Build | ||
|
|
||
| ```shell | ||
| $ forge build | ||
| ``` | ||
|
|
||
| ### Test | ||
|
|
||
| ```shell | ||
| $ forge test | ||
| ``` | ||
|
|
||
| ### Format | ||
|
|
||
| ```shell | ||
| $ forge fmt | ||
| ``` | ||
|
|
||
| ### Gas Snapshots | ||
|
|
||
| ```shell | ||
| $ forge snapshot | ||
| ``` | ||
|
|
||
| ### Anvil | ||
|
|
||
| ```shell | ||
| $ anvil | ||
| ``` | ||
|
|
||
| ### Deploy | ||
|
|
||
| ```shell | ||
| $ forge script script/Counter.s.sol:CounterScript --rpc-url <your_rpc_url> --private-key <your_private_key> | ||
| ``` | ||
|
|
||
| ### Cast | ||
|
|
||
| ```shell | ||
| $ cast <subcommand> | ||
| ``` | ||
|
|
||
| ### Help | ||
|
|
||
| ```shell | ||
| $ forge --help | ||
| $ anvil --help | ||
| $ cast --help | ||
| ``` |
There was a problem hiding this comment.
The README file provides a comprehensive overview of the Foundry toolkit. Consider showing example outputs for command line instructions to enhance clarity and user understanding. Also, consider hyperlinking the bare URL to improve readability and accessibility.
- https://book.getfoundry.sh/
+ [Foundry Documentation](https://book.getfoundry.sh/)- $ forge build
+ $ forge build
+ # Example output: "Compiling..."Tools
Markdownlint
21-21: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
27-27: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
33-33: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
39-39: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
45-45: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
51-51: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
57-57: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
63-63: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
64-64: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
65-65: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
14-14: null (MD034, no-bare-urls)
Bare URL used
DarkaMaul
force-pushed
the
features/test-mutator
branch
from
2af4c66 to
7b983e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
slither/tools/mutator/mutators/LIR.py (2)
Line range hint
41-41: Convert tonot into follow Python best practices.- if not line_no[0] in self.dont_mutate_line: + if line_no[0] not in self.dont_mutate_line:
Line range hint
72-72: Convert tonot into follow Python best practices.- if not line_no[0] in self.dont_mutate_line: + if line_no[0] not in self.dont_mutate_line:
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- .github/scripts/tool_test_runner.sh (1 hunks)
- slither/tools/mutator/main.py (2 hunks)
- slither/tools/mutator/mutators/LIR.py (2 hunks)
- tests/tools/mutator/test_data/test_source_unit/README.md (1 hunks)
- tests/tools/mutator/test_mutator.py (1 hunks)
Files not summarized due to errors (2)
- slither/tools/mutator/main.py: Error: Server error. Please try again later.
- slither/tools/mutator/mutators/LIR.py: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (1)
- tests/tools/mutator/test_data/test_source_unit/README.md
Files skipped from review as they are similar to previous changes (1)
- tests/tools/mutator/test_mutator.py
Additional context used
Ruff
slither/tools/mutator/mutators/LIR.py
41-41: Test for membership should be
not in(E713)Convert to
not in
72-72: Test for membership should be
not in(E713)Convert to
not in
Additional comments not posted (5)
.github/scripts/tool_test_runner.sh (1)
5-5: Ensure the new test directory structure is correctly reflected in the CI pipeline.slither/tools/mutator/mutators/LIR.py (2)
34-34: Ensure that appending "-1" for integer types is consistent with the intended functionality.
66-66: Ensure that appending "-1" for integer types is consistent with the intended functionality.slither/tools/mutator/__main__.py (2)
9-9: Ensure the added imports are used effectively throughout the script.
119-119: Ensure that the function_get_mutatorscorrectly filters and returns the list of mutators based on the provided list.
| status_code=$? | ||
| python -m coverage report | ||
| else | ||
| pytest tests/tools/read-storage/test_read_storage.py | ||
| pytest tests/tools |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate multiple references to 'tests' in the CI configuration files, but none specifically mention 'tests/tools'. This suggests that the new test directory structure might not be fully reflected in the CI pipeline.
.github/workflows/test.yml: Contains several references to 'tests', but none to 'tests/tools'..github/workflows/pylint.yml,.github/workflows/linter.yml,.github/workflows/ci.yml,.github/workflows/black.yml: Also contain references to 'tests', but not 'tests/tools'.
Please ensure that the new test directory structure 'tests/tools' is correctly referenced in the CI configuration files.
- Update CI configuration files to reflect the new test directory structure.
Analysis chain
Ensure the new test directory structure is correctly reflected in the CI pipeline.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new test directory structure is correctly reflected in the CI pipeline.
# Test: Search for the test directory usage in CI configuration files. Expect: Only occurrences of the new structure.
rg --type yaml $'tests/tools' .github/workflows/
Length of output: 47
Script:
#!/bin/bash
# Description: Verify the new test directory structure is correctly reflected in the CI pipeline.
# Test: Search for any references to 'tests' in CI configuration files to ensure the new structure is being used.
rg --type yaml 'tests' .github/workflows/
Length of output: 898
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- tests/tools/mutator/test_mutator.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/tools/mutator/test_mutator.py
We noticed that slither-mutate was using Python 3.10 only syntax, but this was never caught in the CI tests.
This PR fixes this issue and add some test to exercise a better coverage of the tool.
It also uncovered a bug where directories with name ending in
.solwere considered as files by the tool.However, since it is by nature extremely slow,( compiling the code over and over), the main test is disabled and should be used with a debugger when modifying the tool.
The tests could be improved, but that is a nice start here.
Summary by CodeRabbit
New Features
Counterwith functions to set and increment a public state variable.CounterTestfor testing theCountercontract.foundry.tomlfile.Bug Fixes
Tests
slithertool.