test(common): verify coverage gate fails on uncovered change#28
Open
hzj-edu-nju wants to merge 3 commits intobladehan1:developfrom
Open
test(common): verify coverage gate fails on uncovered change#28hzj-edu-nju wants to merge 3 commits intobladehan1:developfrom
hzj-edu-nju wants to merge 3 commits intobladehan1:developfrom
Conversation
Intentional test: add a public utility method to DecodeUtil without any unit test so `diff-cover` reports changed-line coverage below 60% and the Coverage Gate exercises its FAIL path (Acceptance Gate G1.3 — Java PR with coverage below threshold should FAIL). The new method has ~10 executable lines and is not called from production code or tests, so the new lines appear as 0% covered. Revert before merging.
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
common/src/main/java/org/tron/common/utils/DecodeUtil.java (1)
44-47: Redundant zero-branch.
byteLength == 0already yields0viabyteLength * 2, so the explicit branch is unnecessary. Not a functional issue — only relevant if this method is ever kept beyond the sandbox PR.♻️ Proposed simplification
- if (byteLength < 0) { - throw new IllegalArgumentException("byteLength must be non-negative"); - } - if (byteLength == 0) { - return 0; - } - return byteLength * 2; + if (byteLength < 0) { + throw new IllegalArgumentException("byteLength must be non-negative"); + } + return byteLength * 2;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/tron/common/utils/DecodeUtil.java` around lines 44 - 47, In DecodeUtil (the method containing the shown snippet), remove the redundant if (byteLength == 0) branch and simply return byteLength * 2; so replace the conditional block with a single return statement to simplify the code while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/main/java/org/tron/common/utils/DecodeUtil.java`:
- Around line 35-48: Remove the sandbox-only helper method hexLengthForAddress
from DecodeUtil to avoid introducing dead public API; revert the added method
(public static int hexLengthForAddress(int byteLength)) from
org.tron.common.utils.DecodeUtil so callers continue to use existing
ADDRESS_SIZE/ADDRESS_SIZE/2 logic and ensure no tests or usages reference
hexLengthForAddress before merging.
---
Nitpick comments:
In `@common/src/main/java/org/tron/common/utils/DecodeUtil.java`:
- Around line 44-47: In DecodeUtil (the method containing the shown snippet),
remove the redundant if (byteLength == 0) branch and simply return byteLength *
2; so replace the conditional block with a single return statement to simplify
the code while preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a022337-d54f-4fc3-a88b-84c0180b2653
📒 Files selected for processing (1)
common/src/main/java/org/tron/common/utils/DecodeUtil.java
Comment on lines
+35
to
+48
| /** | ||
| * Intentional uncovered helper used to exercise the Coverage Gate FAIL path | ||
| * (changed-line coverage below 60%). No unit test is added on purpose. | ||
| * Revert before merging. | ||
| */ | ||
| public static int hexLengthForAddress(int byteLength) { | ||
| if (byteLength < 0) { | ||
| throw new IllegalArgumentException("byteLength must be non-negative"); | ||
| } | ||
| if (byteLength == 0) { | ||
| return 0; | ||
| } | ||
| return byteLength * 2; | ||
| } |
There was a problem hiding this comment.
Sandbox-only change — ensure this is reverted before merge.
Per the PR description, this method is intentionally added to exercise the Coverage Gate FAIL path and must not be merged. The Javadoc correctly flags this, but it's worth double-checking that once the gate behavior is observed, this PR is closed/reverted and hexLengthForAddress is not left behind as unused public API in common. Since the method has no callers in the tree (existing address-length checks use ADDRESS_SIZE / ADDRESS_SIZE / 2 directly in JsonRpcApiUtil, ValidateAddressServlet, etc.), landing it would just add dead public surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/src/main/java/org/tron/common/utils/DecodeUtil.java` around lines 35 -
48, Remove the sandbox-only helper method hexLengthForAddress from DecodeUtil to
avoid introducing dead public API; revert the added method (public static int
hexLengthForAddress(int byteLength)) from org.tron.common.utils.DecodeUtil so
callers continue to use existing ADDRESS_SIZE/ADDRESS_SIZE/2 logic and ensure no
tests or usages reference hexLengthForAddress before merging.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/src/main/java/org/tron/common/utils/DecodeUtil.java">
<violation number="1" location="common/src/main/java/org/tron/common/utils/DecodeUtil.java:40">
P3: Temporary coverage-gate helper was added as a public production API; remove it or move it to test-only code before merge.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * (changed-line coverage below 60%). No unit test is added on purpose. | ||
| * Revert before merging. | ||
| */ | ||
| public static int hexLengthForAddress(int byteLength) { |
There was a problem hiding this comment.
P3: Temporary coverage-gate helper was added as a public production API; remove it or move it to test-only code before merge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/java/org/tron/common/utils/DecodeUtil.java, line 40:
<comment>Temporary coverage-gate helper was added as a public production API; remove it or move it to test-only code before merge.</comment>
<file context>
@@ -32,4 +32,19 @@ public static boolean addressValid(byte[] address) {
+ * (changed-line coverage below 60%). No unit test is added on purpose.
+ * Revert before merging.
+ */
+ public static int hexLengthForAddress(int byteLength) {
+ if (byteLength < 0) {
+ throw new IllegalArgumentException("byteLength must be non-negative");
</file context>
Add DecodeUtilTest covering all four code paths of addressValid: - null input - empty byte array - wrong-length payload (too short / too long) - wrong address prefix - canonical valid address `addressPreFixByte` is a static mutable field; the test class saves and restores it in @BeforeClass / @afterclass to avoid polluting sibling tests that may run in the same JVM fork.
Append an HTML comment to README.md so this branch becomes a mixed Java + docs PR. Used to verify Acceptance Gate G1.4: diff-cover should still report only the Java src/main/java changed lines (currently 5 lines from hexLengthForAddress) and ignore the docs addition. Revert before merging.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Intentional test PR to exercise Acceptance Gate G1.3 of the coverage gate: a Java PR whose changed-line coverage is
<= 60%should fail the gate.The PR adds a new public utility method
DecodeUtil.hexLengthForAddress(int)with ~10 executable lines. No unit test is added on purpose, sodiff-coverwill report0%changed-line coverage and the Coverage Gate job should exit non-zero withChanged-line Gate: FAIL (<= 60%).Expected CI signals
Coverage Gatestep summary shows:Changed-line Coverage: 0%Changed-line Gate: FAIL (<= 60%)Overall Delta Gate: PASS (>= -0.1%)(no regression in overall INSTRUCTION delta)Coverage Gatejob ends withCoverage gate failed: changed-line coverage must be > 60%and exit code1.Not for merge
This PR is a sandbox test and must not be merged. Revert / close after observing the expected gate FAIL.
Summary by cubic
Adds an uncovered public helper
DecodeUtil.hexLengthForAddress(int)to intentionally trigger Coverage Gate G1.3 (changed-line coverage <= 60% should fail), and adds tests forDecodeUtil.addressValidonly. Also adds a README HTML comment to verify G1.4 (mixed PRs ignore docs lines), so CI should report 0% changed-line coverage on the new Java lines and fail the changed-line gate—do not merge.Written for commit 6258af8. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests
Documentation