Repository navigation
ci(common): coverage easy - #5
Merged
Merged
Conversation
…ronprotocol#6586) * test: add dual DB engine (LevelDB + RocksDB) test coverage
run jacocoTestReport once at root to execute all subprojects upload all module jacoco xml files as a single artifact build madrapps paths dynamically from downloaded xml files
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage Report
Files
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 50 files
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="framework/src/main/java/org/tron/common/logsfilter/nativequeue/NativeMessageQueue.java">
<violation number="1" location="framework/src/main/java/org/tron/common/logsfilter/nativequeue/NativeMessageQueue.java:62">
P2: Resetting `instance` here is not thread-safe with the current double-checked access pattern. Without `volatile` (or fully synchronized reads), threads may miss this null assignment and keep using a stale stopped singleton.</violation>
</file>
<file name=".github/workflows/pr-cancel.yml">
<violation number="1" location=".github/workflows/pr-cancel.yml:41">
P2: The SHA equality check only cancels runs for the latest commit, so older in-flight runs from the same PR can keep running after the PR is closed.</violation>
</file>
<file name="framework/src/test/java/org/tron/common/storage/DbDataSourceImplTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/storage/DbDataSourceImplTest.java:173">
P2: `closeDB()` is unreachable in the expected-exception path; close the datasource in a `finally` block so resources are always released.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/db/backup/NeedBeanCondition.java">
<violation number="1" location="framework/src/main/java/org/tron/core/db/backup/NeedBeanCondition.java:12">
P3: `Args.getInstance() == null` is dead code here because `getInstance()` always returns a non-null singleton instance.</violation>
</file>
<file name=".github/workflows/pr-build.yml">
<violation number="1" location=".github/workflows/pr-build.yml:85">
P1: Add `contents: read` to the `coverage` job permissions; otherwise checkout/repository reads can fail because only `pull-requests: write` is granted.</violation>
</file>
<file name=".github/workflows/codeql.yml">
<violation number="1" location=".github/workflows/codeql.yml:35">
P2: Pin GitHub Actions to immutable commit SHAs instead of major tags to reduce workflow supply-chain risk.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:842">
P1: RocksDB initialization is now unconditional, so LevelDB configurations can fail at startup by loading RocksDB native libs unnecessarily.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.java:115">
P2: Receive-validation assertions are overly permissive and can pass on unrelated Merkle-root failures.</violation>
<violation number="2" location="framework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.java:122">
P2: Spend-signature tests now accept unrelated root-validation errors, weakening regression detection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
P1: Add contents: read to the coverage job permissions; otherwise checkout/repository reads can fail because only pull-requests: write is granted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/pr-build.yml, line 85:
<comment>Add `contents: read` to the `coverage` job permissions; otherwise checkout/repository reads can fail because only `pull-requests: write` is granted.</comment>
<file context>
@@ -0,0 +1,123 @@
+ runs-on: ubuntu-latest
+ timeout-minutes: 5
+ permissions:
+ pull-requests: write
+
+ steps:
</file context>
Comment on lines
+842
to
+843
| initRocksDbBackupProperty(config); | ||
| initRocksDbSettings(config); |
There was a problem hiding this comment.
P1: RocksDB initialization is now unconditional, so LevelDB configurations can fail at startup by loading RocksDB native libs unnecessarily.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/Args.java, line 842:
<comment>RocksDB initialization is now unconditional, so LevelDB configurations can fail at startup by loading RocksDB native libs unnecessarily.</comment>
<file context>
@@ -784,11 +839,8 @@ public static void applyConfigParams(
- initRocksDbBackupProperty(config);
- initRocksDbSettings(config);
- }
+ initRocksDbBackupProperty(config);
+ initRocksDbSettings(config);
</file context>
Suggested change
| initRocksDbBackupProperty(config); | |
| initRocksDbSettings(config); | |
| if (Constant.ROCKSDB.equalsIgnoreCase(PARAMETER.storage.getDbEngine()) || Arch.isArm64()) { | |
| initRocksDbBackupProperty(config); | |
| initRocksDbSettings(config); | |
| } |
| context = null; | ||
| } | ||
| synchronized (NativeMessageQueue.class) { | ||
| instance = null; |
There was a problem hiding this comment.
P2: Resetting instance here is not thread-safe with the current double-checked access pattern. Without volatile (or fully synchronized reads), threads may miss this null assignment and keep using a stale stopped singleton.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/common/logsfilter/nativequeue/NativeMessageQueue.java, line 62:
<comment>Resetting `instance` here is not thread-safe with the current double-checked access pattern. Without `volatile` (or fully synchronized reads), threads may miss this null assignment and keep using a stale stopped singleton.</comment>
<file context>
@@ -51,10 +51,15 @@ public boolean start(int bindPort, int sendQueueLength) {
+ context = null;
+ }
+ synchronized (NativeMessageQueue.class) {
+ instance = null;
}
}
</file context>
|
|
||
| for (const run of runs) { | ||
| const isTargetPr = !run.pull_requests?.length || run.pull_requests.some((pr) => pr.number === prNumber); | ||
| if (run.head_sha === headSha && isTargetPr) { |
There was a problem hiding this comment.
P2: The SHA equality check only cancels runs for the latest commit, so older in-flight runs from the same PR can keep running after the PR is closed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/pr-cancel.yml, line 41:
<comment>The SHA equality check only cancels runs for the latest commit, so older in-flight runs from the same PR can keep running after the PR is closed.</comment>
<file context>
@@ -0,0 +1,51 @@
+
+ for (const run of runs) {
+ const isTargetPr = !run.pull_requests?.length || run.pull_requests.some((pr) => pr.number === prNumber);
+ if (run.head_sha === headSha && isTargetPr) {
+ await github.rest.actions.cancelWorkflowRun({
+ owner: context.repo.owner,
</file context>
| rows.put(key1.getBytes(), value1.getBytes()); | ||
| rows.put(key2.getBytes(), value2.getBytes()); | ||
|
|
||
| dataSource.updateByBatch(rows); |
There was a problem hiding this comment.
P2: closeDB() is unreachable in the expected-exception path; close the datasource in a finally block so resources are always released.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/storage/DbDataSourceImplTest.java, line 173:
<comment>`closeDB()` is unreachable in the expected-exception path; close the datasource in a `finally` block so resources are always released.</comment>
<file context>
@@ -0,0 +1,454 @@
+ rows.put(key1.getBytes(), value1.getBytes());
+ rows.put(key2.getBytes(), value2.getBytes());
+
+ dataSource.updateByBatch(rows);
+
+ assertEquals("50000", ByteArray.toStr(dataSource.getData(key1.getBytes())));
</file context>
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| uses: actions/checkout@v5 |
There was a problem hiding this comment.
P2: Pin GitHub Actions to immutable commit SHAs instead of major tags to reduce workflow supply-chain risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/codeql.yml, line 35:
<comment>Pin GitHub Actions to immutable commit SHAs instead of major tags to reduce workflow supply-chain risk.</comment>
<file context>
@@ -29,16 +32,23 @@ jobs:
steps:
- name: Checkout repository
- uses: actions/checkout@v4
+ uses: actions/checkout@v5
# Initializes the CodeQL tools for scanning.
</file context>
Comment on lines
+122
to
+125
| private static final Set<String> SPEND_VALIDATION_ERRORS = new HashSet<>(Arrays.asList( | ||
| "librustzcashSaplingCheckSpend error", | ||
| "Rt is invalid." | ||
| )); |
There was a problem hiding this comment.
P2: Spend-signature tests now accept unrelated root-validation errors, weakening regression detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.java, line 122:
<comment>Spend-signature tests now accept unrelated root-validation errors, weakening regression detection.</comment>
<file context>
@@ -105,6 +109,21 @@
+ ));
+
+ // Valid error messages when spend description or signature is wrong.
+ private static final Set<String> SPEND_VALIDATION_ERRORS = new HashSet<>(Arrays.asList(
+ "librustzcashSaplingCheckSpend error",
+ "Rt is invalid."
</file context>
Suggested change
| private static final Set<String> SPEND_VALIDATION_ERRORS = new HashSet<>(Arrays.asList( | |
| "librustzcashSaplingCheckSpend error", | |
| "Rt is invalid." | |
| )); | |
| private static final Set<String> SPEND_VALIDATION_ERRORS = new HashSet<>(Arrays.asList( | |
| "librustzcashSaplingCheckSpend error" | |
| )); |
| // Valid error messages when receive description fields are missing or wrong. | ||
| // The exact message depends on which native validation check fails first, | ||
| // which varies with merkle tree state and execution order. | ||
| private static final Set<String> RECEIVE_VALIDATION_ERRORS = new HashSet<>(Arrays.asList( |
There was a problem hiding this comment.
P2: Receive-validation assertions are overly permissive and can pass on unrelated Merkle-root failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/zksnark/ShieldedReceiveTest.java, line 115:
<comment>Receive-validation assertions are overly permissive and can pass on unrelated Merkle-root failures.</comment>
<file context>
@@ -105,6 +109,21 @@
+ // Valid error messages when receive description fields are missing or wrong.
+ // The exact message depends on which native validation check fails first,
+ // which varies with merkle tree state and execution order.
+ private static final Set<String> RECEIVE_VALIDATION_ERRORS = new HashSet<>(Arrays.asList(
+ "param is null",
+ "Rt is invalid.",
</file context>
| @Override | ||
| public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) { | ||
| return ("ROCKSDB".equals(Args.getInstance().getStorage().getDbEngine().toUpperCase())) | ||
| if (Args.getInstance() == null || Args.getInstance().getStorage() == null |
There was a problem hiding this comment.
P3: Args.getInstance() == null is dead code here because getInstance() always returns a non-null singleton instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/db/backup/NeedBeanCondition.java, line 12:
<comment>`Args.getInstance() == null` is dead code here because `getInstance()` always returns a non-null singleton instance.</comment>
<file context>
@@ -9,7 +9,12 @@ public class NeedBeanCondition implements Condition {
@Override
public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) {
- return ("ROCKSDB".equals(Args.getInstance().getStorage().getDbEngine().toUpperCase()))
+ if (Args.getInstance() == null || Args.getInstance().getStorage() == null
+ || Args.getInstance().getStorage().getDbEngine() == null
+ || Args.getInstance().getDbBackupConfig() == null) {
</file context>
Suggested change
| if (Args.getInstance() == null || Args.getInstance().getStorage() == null | |
| if (Args.getInstance().getStorage() == null |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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=".github/workflows/pr-build.yml">
<violation number="1" location=".github/workflows/pr-build.yml:7">
P2: The new `pull_request.paths` allowlist is too restrictive and prevents this workflow from running for CI-only changes (like `.github/workflows/**`), leaving workflow updates unvalidated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
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
4 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.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Adds PR build and coverage reporting with unified JaCoCo across modules, plus faster CI via refined path filters. Also upgrades tests for dual DB engine coverage and reduces flakes, while deprecating several CLI flags in favor of config keys.
New Features
pr-build.ymlto build on PRs (Debian 11/JDK 8), run tests, aggregate JaCoCo XMLs, and upload coverage; added Java/Gradle path allowlist and refinedpaths-ignoreacrosspr-build.yml,system-test.yml, andcodeql.ymlto skip non-code changes; addedpr-cancel.ymlto stop in-progress runs on PR close.codecov.yml(comment disabled) and consolidated JaCoCo inframework/build.gradlewith retries, memory tuning, and Windows test excludes;pr-check.ymltrimmed to validation/checkstyle and updated toactions/github-script@v8/actions/upload-artifact@v6; CI actions bumped (actions/checkout@v5,actions/setup-java@v5),codeql.ymlswitched to manual build on JDK 8;math-check.ymlupgraded toactions/checkout@v5,actions/upload-artifact@v6, andactions/github-script@v8.Refactors
BaseMethodTestfor per-test Spring context isolation; broadened LevelDB/RocksDB coverage via system property overrides and assumptions; reduced flakiness (e.g., documented trie instability) and addedDbDataSourceImplTest.Args/CLIParameterwith mappings to config keys; addedevent.subscribe.enablewith defaults in config; removed arm64-specific DB engine defaulting; strengthenedNeedBeanConditionnull checks; improvedNativeMessageQueue.stop()to release resources fully; fixed test port probing inPublicMethod.Written for commit 5945543. Summary will update on new commits.