fix: coverage script breaking after enabling viaIR #2082
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaybuidl
added
Type: Maintenance 🚧
Type: Toolchain ⚒️
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as contracts/scripts/coverage.sh
participant Env as Environment
participant Build as yarn build
participant HHCfg as hardhat.config.ts
participant Coverage as npx hardhat coverage
Dev->>Script: run coverage.sh
Script->>Env: export VIA_IR=false
Script->>Build: yarn build
Build->>HHCfg: hardhat loads config (reads process.env.VIA_IR)
HHCfg->>Build: viaIR = false (VIA_IR="false")
Script->>Coverage: npx hardhat coverage
Coverage->>HHCfg: uses solc viaIR=false and solcover config (irMinimum: true)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jaybuidl
force-pushed
the
fix/coverage-script-again-ffs
branch
from
b021221 to
7e107cc
Compare
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/scripts/coverage.sh (1)
32-32: Use the same portable sed helper consistentlyThis invocation uses
-i -e, which breaks on BSD sed. Reuse thesedihelper for consistency and portability.- sed -i -e 's/\/.*\/kleros-v2\/contracts\///g' coverage/lcov-hardhat.info + sedi 's/\/.*\/kleros-v2\/contracts\///g' coverage/lcov-hardhat.info
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/.solcover.js(1 hunks)contracts/scripts/coverage.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: contracts-testing
🔇 Additional comments (1)
contracts/.solcover.js (1)
10-10: Confirmed:irMinimumis a supported solidity-coverage optionThe
irMinimumboolean flag is recognized and defaults tofalse. It can speed up test runs when using solc’s viaIR mode but may cause compilation failures in some setups—enable it only if your project still compiles under coverage with viaIR.No changes required.
contracts/scripts/coverage.sh
Outdated
| fi | ||
|
|
||
| # Generate the Hardhat coverage report | ||
| sed -i '' 's/\(viaIR: \)true/\1false/' hardhat.config.ts |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the viaIR toggle robust, cross-platform, and reversible
Current sed:
- May fail across macOS/Linux due to
-i ''usage. - Matches only
viaIR: truewith a single space; misses variations like extra spaces. - Permanently mutates hardhat.config.ts (no revert), leaving a dirty tree locally.
Suggestions:
- Use a cross-platform sed wrapper.
- Match flexible whitespace.
- Revert the change on exit with a trap.
Apply within this hunk:
- sed -i '' 's/\(viaIR: \)true/\1false/' hardhat.config.ts
+ # Disable viaIR during coverage with a portable sed and auto-restore on exit
+ sedi -E 's/(viaIR:[[:space:]]*)true/\1false/' hardhat.config.ts
+ trap 'sedi -E "s/(viaIR:[[:space:]]*)false/\1true/" hardhat.config.ts' EXITAdd this helper near the top of the script (outside this hunk):
# Portable in-place sed (GNU vs BSD)
sedi() {
if sed --version >/dev/null 2>&1; then
sed -i -E "$@"
else
sed -i '' -E "$@"
fi
}Alternative (cleaner): avoid sed entirely by toggling viaIR in hardhat.config.ts when process.env.COVERAGE is set by the coverage task.
🤖 Prompt for AI Agents
In contracts/scripts/coverage.sh around line 22, the current sed call is
non-portable, only matches a single-space pattern, and permanently modifies
hardhat.config.ts; replace it by adding a portable sedi helper near the top of
the script (outside this hunk) that detects GNU vs BSD sed, then use that helper
to run a regex with flexible whitespace matching (e.g. match viaIR:\s*true) to
toggle true->false, create a temporary backup or copy before modifying, and
install a trap to restore the original file on EXIT (and on ERR/INT) so the
change is reversible and leaves no dirty tree; alternatively mention switching
to an environment-variable-based toggle in hardhat.config.ts and avoid sed
entirely.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/hardhat.config.ts (1)
31-31: Make VIA_IR parsing more robust (accept common “falsey” values, case-insensitive)Today only the exact string "false" disables IR. Consider accepting common variants and casing (e.g., 0, off, no, FALSE), while still defaulting to true.
- viaIR: process.env.VIA_IR !== "false", // Defaults to true + // Defaults to true unless explicitly disabled + viaIR: !["false", "0", "off", "no"].includes((process.env.VIA_IR ?? "").toLowerCase()),contracts/scripts/coverage.sh (1)
24-27: Scope VIA_IR to the commands instead of exporting it globallyInline the env var per command so it won’t leak if the script is sourced or grows later. Nice improvement moving away from sed—this avoids the prior cross-platform pitfalls.
- export VIA_IR=false - yarn build + VIA_IR=false yarn build echo "Running Hardhat coverage..." - yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --show-stack-traces --testfiles "test/**/*.ts" + VIA_IR=false yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --show-stack-traces --testfiles "test/**/*.ts"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/hardhat.config.ts(1 hunks)contracts/scripts/coverage.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: SonarCloud
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
jaybuidl
force-pushed
the
fix/coverage-script-again-ffs
branch
from
464568e to
b8628bb
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR focuses on enhancing the configuration for Solidity contract coverage in a Hardhat project. It introduces new environment variable handling and adjustments to the optimizer settings.
Detailed summary
contracts/.solcover.js: AddedirMinimum: true.contracts/hardhat.config.ts: ChangedviaIRto depend onprocess.env.VIA_IR.contracts/scripts/coverage.sh: Addedexport VIA_IR=falsebefore running coverage commands.Summary by CodeRabbit
Tests
Chores