Add test detection and documentation#33
Conversation
- Info about runnables.scm - How to link tasks to runnables via tags - How to run tasks via keyboard shortcut
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a README "Tasks" section, a Forge Solidity test contract, Tree‑sitter runnable queries for Solidity test discovery, and a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor
participant Scanner
participant Tasks
participant ForgeCLI
participant TestVM
Editor->>Scanner: Open/scan Solidity file
Scanner-->>Editor: Report run-capabilities (solidity-test / solidity-contract-test)
Editor->>Tasks: Trigger test task (keymap or run command)
Tasks->>ForgeCLI: Invoke `forge test` with selector (function/contract/file/all)
ForgeCLI->>TestVM: Execute tests (vm, expectEmit, expectRevert, fuzz)
TestVM-->>ForgeCLI: Return results (pass/fail, events, reverts)
ForgeCLI-->>Editor: Present test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)code/example.test.sol(1 hunks)languages/solidity/runnables.scm(1 hunks)
🔇 Additional comments (3)
languages/solidity/runnables.scm (1)
12-17: LGTM!The contract-wide test detection pattern correctly captures contract names and applies the appropriate tag for running all tests in a contract.
code/example.test.sol (2)
34-103: Excellent test coverage and structure.The test suite demonstrates comprehensive testing patterns including:
- State verification after operations
- Event emission testing with
vm.expectEmit- Access control testing with
vm.prank- Custom error handling with
abi.encodeWithSelector- Fuzz testing with proper input constraints
The tests are well-organized and follow clear naming conventions.
1-6: Import path is valid.The contract file exists at
code/example.sol, so the import statement is correct.
mikalsande
force-pushed
the
runnables
branch
from
5fd3f11 to
69cdeee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/example.test.sol (3)
27-50: Good coverage of initial state and transfer + event behavior
test_InitialState,test_Transfer, andtest_Transfer_EmitEventcollectively check supply, balances, and theTransferevent, and thevm.expectEmitpattern is idiomatic. One optional improvement would be to also asserttoken.balanceOf(owner)intest_Transfer_EmitEventto keep a pure “state + event” check in that test as well.
52-70: Mint tests are clear; consider verifying prank/expectRevert behavior
test_Mint_AsOwnerandtest_RevertWhen_MintNotOwnernicely validate both the happy path and the onlyOwner guard, including the revert message. Depending on Foundry’s semantics forvm.prankandvm.expectRevert, it can be safer to structure the sequence so it’s obvious the prank applies to the actualmintcall, e.g.:- vm.prank(alice); - - // Expect the "Not the owner" require message from onlyOwner modifier - vm.expectRevert("Not the owner"); - token.mint(alice, amount); + // Expect the "Not the owner" require message from onlyOwner modifier + vm.prank(alice); + vm.expectRevert("Not the owner"); + token.mint(alice, amount);This makes the intent explicit and avoids any ambiguity about which call the prank targets.
95-102: Fuzz test is useful; consider asserting more invariantsThe fuzzed
testFuzz_Transfercorrectly constrainsamountand checks Alice’s balance. You might optionally extend it to also assert invariants liketoken.balanceOf(owner) == INITIAL_SUPPLY - amountor thattotalSupply()remains constant to get extra confidence from each fuzz case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/example.test.sol(1 hunks)languages/solidity/runnables.scm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- languages/solidity/runnables.scm
🔇 Additional comments (2)
code/example.test.sol (2)
7-25: Fixture setup and owner wiring look solidState variables,
INITIAL_SUPPLY, andsetUpcorrectly align so thatowneris the deployer and holds the full initial supply; this is a clean, reusable fixture for the rest of the tests.
72-93: Burn tests correctly exercise success and custom-error failure paths
test_Burnandtest_RevertWhen_BurnInsufficientBalancevalidate both balance/supply reduction and the encodedInsufficientBalancecustom error. The explicitabi.encodeWithSelectorusage is precise and keeps the test tightly coupled to the contract’s error signature.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.zed/tasks.json(1 hunks)README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (3)
.zed/tasks.json (3)
17-48: Foundry/Forge tasks look solid.Clear differentiation between symbol, contract, file, and all-tests execution using appropriate
forgeflags. Since these are framework-tested per the PR description, no concerns.
53-84: Verify intent: Hardhat symbol vs contract tasks use identical commands.Lines 54-60 and 62-68 both execute
hardhat test --grep $ZED_SYMBOL, but have different labels and tags ("symbol" vs "contract"). If intended, this works, but if the contract task should match contracts instead of test names, the command differs from the Foundry pattern (which uses--match-contract). Same pattern appears in Truffle (lines 90–104) and Brownie (lines 126–140).Confirm these are intentionally identical or update contract tasks with framework-specific contract-filtering flags.
161-192: Clarify intent: Slither symbol and contract tasks use identical commands.Lines 162–168 and 170–176 both run
slither $ZED_FILEwith identical args. If intended (to allow different keybinds via tags), this mirrors the Hardhat/Truffle/Brownie pattern. If not intentional, differentiate with framework-specific arguments (e.g., contract name filtering if slither supports it).Aligns with similar pattern flagged in Hardhat/Truffle/Brownie review.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Thank you for your contribution! it's not clear to me how to use this, could you provide a screenshot or an example of the feature this adds? @czepluch are you able to test this? |
|
I can give it a look a bit later today or tomorrow. |
|
Hey, so sorry about the slow response. Was starting a new job and things have been a bit hectic. Taking a look now. |
|
Thanks for working on this! I tested the extension locally and found a couple of issues that prevented the play buttons from appearing. I pushed a fix to your branch: Tag mismatch: The tags in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.zed/tasks.json (1)
53-68: Hardhat symbol and contract tasks are identical.Both tasks use
--grep $ZED_SYMBOLwith the same arguments, making them functionally identical. Unlike Foundry which has distinct--match-testvs--match-contractflags, Hardhat's--grepdoesn't differentiate between test function names and contract names.Consider either:
- Removing the redundant
(contract)variant, or- Adding a comment clarifying they exist for tag-based discovery purposes
The same pattern applies to Truffle (lines 89-104) and Brownie (lines 125-140).
♻️ Option: Consolidate to single symbol task
{ "label": "hardhat test: $ZED_SYMBOL", "command": "npx", "args": ["hardhat", "test", "--grep", "$ZED_SYMBOL"], - "tags": ["solidity-test"], + "tags": ["solidity-test", "solidity-contract-test"], "reveal": "always", "use_new_terminal": false }, - { - "label": "hardhat test: $ZED_SYMBOL (contract)", - "command": "npx", - "args": ["hardhat", "test", "--grep", "$ZED_SYMBOL"], - "tags": ["solidity-contract-test"], - "reveal": "always", - "use_new_terminal": false - },
- Fix tasks.json tags to match runnables.scm tags (solidity-test, solidity-contract-test). Previously tags like ["foundry", "test", "symbol"] didn't match any runnable, so no play buttons appeared. - Restructure runnables.scm query to follow the pattern used by Zed's built-in language extensions. - Broaden test function regex from ^(test_) to ^test to match all Solidity test conventions: test_, testFuzz_, testFail_, testFork_. - Add separate invariant pattern for invariant test detection. - Consolidate duplicate security/linting tasks that were identical.
|
@mikalsande can you please enable verified commits? |
|
Can you please pump the version @mikalsande? |
3 participants
Add runnables.scm for detecting tests and add examples to the README for how to connect tasks with tags from runnables.scm and how to add keyboard shortcut to run the test under the cursor.
The foundry tasks have been tested, the others were generated so they may or may not work. They serve as useful examples though.
When testing it if you get
Nothing to compilewhen running a test via the play button in the UI or the keyboard shortcut it means the mapping is working 👍 To get things to actually work we'd have to set up Foundry in the repo and I'm not sure that is helpful since there are many testing frameworks for Solidity.Summary by CodeRabbit
Documentation
Examples
New Features