-
Notifications
You must be signed in to change notification settings - Fork 3
test(DEVoterVoting): add test for negative duration validation #103
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.
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
Conversation
Caution Review failedThe pull request is closed. WalkthroughRemoved a single line comment in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/DEVoterVoting.ts (3)
12-12
: LGTM: rely on hardhat-viem types; no manual HRE augmentation needed.
Nice cleanup. Optional: the explicit import of "@nomicfoundation/hardhat-viem/types" is usually redundant if the plugin is loaded in hardhat.config; consider dropping it to reduce noise.
90-90
: Cast startTimeBefore to Number to avoid bigint/number mismatch.
If time.latest() returns bigint in the viem helpers, closeTo will complain. Cast explicitly:- expect(Number(votingStartTime)).to.be.closeTo(startTimeBefore, 5); + expect(Number(votingStartTime)).to.be.closeTo(Number(startTimeBefore), 5);
93-112
: Rename test, use 0n, and assert generic rejection (Viem lacks hardhat-chai-matchers)Viem-based Hardhat toolbox doesn't expose hardhat-chai-matchers (no .revertedWithCustomError). Rename the test to "non‑positive", prefer 0n, and assert .to.be.rejected.
-it("Should reject start with negative duration", async function () { +it("Should reject start with non-positive duration", async function () { @@ - // Note: In Solidity, we can't directly pass negative numbers as uint256 - // But we can test the requirement that duration must be greater than zero - // by using a very small duration that will be rejected + // Solidity uint256 can't be negative; enforce > 0 by passing 0 to hit the same check. @@ - await expect( - devoterVoting.write.startVotingPeriod([BigInt(0)], { - account: owner.account, - }) - ).to.be.rejectedWith("Invalid duration"); + await expect( + devoterVoting.write.startVotingPeriod([0n], { account: owner.account }) + ).to.be.rejected; // hardhat-viem doesn't expose .revertedWithCustomError
f26c4fa
to
07ff670
Compare
Add test case to verify that voting period cannot be started with invalid (zero/negative) duration
Summary by CodeRabbit
Tests
Chores