Repository navigation
ci(common): code coverage - #4
Merged
Merged
Conversation
…ronprotocol#6586) * test: add dual DB engine (LevelDB + RocksDB) test coverage
docs: fix shieldedTransaction typos in comments
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 |
![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/test/java/org/tron/common/BaseMethodTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/BaseMethodTest.java:80">
P2: `beforeDestroy()` is called even when initialization failed, so subclass overrides that access `dbManager`, `context`, etc. will NPE and mask the real test failure. Move the call inside the `context != null` guard.</violation>
</file>
<file name=".github/workflows/pr-cancel.yml">
<violation number="1" location=".github/workflows/pr-cancel.yml:42">
P2: Wrap `cancelWorkflowRun` in a try/catch. If a run finishes between the list and cancel calls, the API returns 409, which throws and aborts the rest of the loop—leaving other in-progress runs uncancelled.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/db2/ChainbaseTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/db2/ChainbaseTest.java:23">
P2: `ChainbaseTest` doesn't use any Spring beans (`context`, `appT`, `dbManager`, `chainBaseManager`) from `BaseMethodTest`, yet extending it spins up and tears down a full `TronApplicationContext` per test method. This contradicts the base class's own guideline: *"Tests that don't need Spring should NOT extend either base class."* Consider a lighter-weight base that only provides `TemporaryFolder` + `Args.setParam`/`clearParam` lifecycle, or inline the setup.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/config/args/ArgsTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/config/args/ArgsTest.java:357">
P2: Tautological assertion: the expected value mirrors the production code's logic. If the code reads `System.getProperty("storage.db.engine")` to pick the engine, this test will always pass regardless of correctness. Consider controlling the environment instead — e.g., save and clear the system property before the test, assert `"LEVELDB"`, then restore it — so the test validates a concrete behavior rather than echoing the code path.</violation>
</file>
<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">
P1: Setting `instance = null` in `stop()` introduces a concurrency bug because `instance` is not `volatile`. After `stop()` runs, other threads calling `getInstance()` can still see the stale (stopped) instance at the first unsynchronized null-check, skipping the synchronized block entirely and returning a dead singleton.
The field must be declared `volatile` for the double-checked locking + reset pattern to work correctly under the Java Memory Model.</violation>
</file>
<file name="framework/src/test/java/org/tron/common/utils/PublicMethod.java">
<violation number="1" location="framework/src/test/java/org/tron/common/utils/PublicMethod.java:347">
P2: `setReuseAddress(true)` is called after the `ServerSocket(port)` constructor already bound the socket, so it has no effect. Use the no-arg constructor and bind after setting the option, otherwise the port may not be immediately reusable after this check closes the socket.</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:157">
P2: `dataSource.closeDB()` is unreachable because the preceding `dataSource.updateByBatch(rows)` is expected to throw a `RuntimeException` (via `exception.expect()`). The database resource will never be closed. Consider moving `closeDB()` before the `exception.expect()` call or using a try-finally / `@After` to ensure cleanup.</violation>
</file>
<file name="framework/build.gradle">
<violation number="1" location="framework/build.gradle:109">
P3: Closure parameter is typed as `Task` but uses `Test`-specific APIs (`retry`, `exclude`, `maxHeapSize`, `forkEvery`, `jvmArgs`). This works only because Gradle uses dynamic Groovy dispatch. Type it as `Test` for correctness and better IDE support.</violation>
</file>
<file name=".github/workflows/pr-build.yml">
<violation number="1" location=".github/workflows/pr-build.yml:214">
P1: Version mismatch: artifacts are uploaded with `upload-artifact@v6` but downloaded with `download-artifact@v4`. These actions must use compatible major versions to share the same artifact backend. This will likely cause the `coverage` job to fail at the download step.</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.
| } else { | ||
| Assert.assertEquals("LEVELDB", parameter.getStorage().getDbEngine()); | ||
| } | ||
| String expectedEngine = System.getProperty("storage.db.engine") != null |
There was a problem hiding this comment.
P2: Tautological assertion: the expected value mirrors the production code's logic. If the code reads System.getProperty("storage.db.engine") to pick the engine, this test will always pass regardless of correctness. Consider controlling the environment instead — e.g., save and clear the system property before the test, assert "LEVELDB", then restore it — so the test validates a concrete behavior rather than echoing the code path.
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/config/args/ArgsTest.java, line 357:
<comment>Tautological assertion: the expected value mirrors the production code's logic. If the code reads `System.getProperty("storage.db.engine")` to pick the engine, this test will always pass regardless of correctness. Consider controlling the environment instead — e.g., save and clear the system property before the test, assert `"LEVELDB"`, then restore it — so the test validates a concrete behavior rather than echoing the code path.</comment>
<file context>
@@ -333,11 +354,9 @@ public void testConfigStorageDefaults() {
- } else {
- Assert.assertEquals("LEVELDB", parameter.getStorage().getDbEngine());
- }
+ String expectedEngine = System.getProperty("storage.db.engine") != null
+ ? System.getProperty("storage.db.engine") : "LEVELDB";
+ Assert.assertEquals(expectedEngine, parameter.getStorage().getDbEngine());
</file context>
| socket.getPort(); | ||
| } catch (IOException e) { | ||
| try (java.net.ServerSocket ss = new java.net.ServerSocket(port)) { | ||
| ss.setReuseAddress(true); |
There was a problem hiding this comment.
P2: setReuseAddress(true) is called after the ServerSocket(port) constructor already bound the socket, so it has no effect. Use the no-arg constructor and bind after setting the option, otherwise the port may not be immediately reusable after this check closes the socket.
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/utils/PublicMethod.java, line 347:
<comment>`setReuseAddress(true)` is called after the `ServerSocket(port)` constructor already bound the socket, so it has no effect. Use the no-arg constructor and bind after setting the option, otherwise the port may not be immediately reusable after this check closes the socket.</comment>
<file context>
@@ -343,13 +343,11 @@ public static int chooseRandomPort(int min, int max) {
- socket.getPort();
- } catch (IOException e) {
+ try (java.net.ServerSocket ss = new java.net.ServerSocket(port)) {
+ ss.setReuseAddress(true);
return true;
+ } catch (IOException e) {
</file context>
| assertEquals(new ArrayList<>(), doGetKeysNext(dataSource, key1, 0)); | ||
| assertEquals(Sets.newHashSet(), doGetValuesNext(dataSource, key1, 0)); | ||
| assertEquals(Sets.newHashSet(), getlatestValues(dataSource, 0)); | ||
| dataSource.closeDB(); |
There was a problem hiding this comment.
P2: dataSource.closeDB() is unreachable because the preceding dataSource.updateByBatch(rows) is expected to throw a RuntimeException (via exception.expect()). The database resource will never be closed. Consider moving closeDB() before the exception.expect() call or using a try-finally / @After to ensure cleanup.
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 157:
<comment>`dataSource.closeDB()` is unreachable because the preceding `dataSource.updateByBatch(rows)` is expected to throw a `RuntimeException` (via `exception.expect()`). The database resource will never be closed. Consider moving `closeDB()` before the `exception.expect()` call or using a try-finally / `@After` to ensure cleanup.</comment>
<file context>
@@ -0,0 +1,454 @@
+ assertEquals(new ArrayList<>(), doGetKeysNext(dataSource, key1, 0));
+ assertEquals(Sets.newHashSet(), doGetValuesNext(dataSource, key1, 0));
+ assertEquals(Sets.newHashSet(), getlatestValues(dataSource, 0));
+ dataSource.closeDB();
+ }
+
</file context>
|
|
||
| test { | ||
| retry { | ||
| def configureTestTask = { Task t -> |
There was a problem hiding this comment.
P3: Closure parameter is typed as Task but uses Test-specific APIs (retry, exclude, maxHeapSize, forkEvery, jvmArgs). This works only because Gradle uses dynamic Groovy dispatch. Type it as Test for correctness and better IDE support.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/build.gradle, line 109:
<comment>Closure parameter is typed as `Task` but uses `Test`-specific APIs (`retry`, `exclude`, `maxHeapSize`, `forkEvery`, `jvmArgs`). This works only because Gradle uses dynamic Groovy dispatch. Type it as `Test` for correctness and better IDE support.</comment>
<file context>
@@ -106,19 +106,38 @@ run {
-test {
- retry {
+def configureTestTask = { Task t ->
+ t.retry {
maxRetries = 5
</file context>
Suggested change
| def configureTestTask = { Task t -> | |
| def configureTestTask = { Test t -> |
Code Coverage Report
Files
|
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 `paths` allowlist is too narrow: workflow/config-only PRs won’t trigger this pipeline, so broken CI coverage changes can land 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
6 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 code coverage with a multi-OS/JDK matrix and aggregates JaCoCo across modules to show coverage on pull requests. Also tightens CI with refined path filters, newer actions, per-PR concurrency, cancellation on PR close, and separates builds from checks; improves test reliability and config safety.
New Features
pr-buildworkflow (macOS/Linux matrix, JDK 8/11/17) to build, test, and upload aggregated JaCoCo XML; root aggregation andcodecov.ymladded (comments disabled); supportsworkflow_dispatchto run a single target.pr-check,codeql,math-check, andsystem-testwith refinedpaths-ignore, per-PR concurrency, latest actions (checkout@v5,setup-java@v5,upload-artifact@v6,github-script@v8), and manual CodeQL build on JDK 8; moved build duties frompr-checktopr-build.pr-cancelto stop in-progresspr-build,system-test, andcodeqlruns when a PR closes.Refactors
BaseMethodTestfor per-method Spring isolation; unified DB tests intoDbDataSourceImplTest; added dual LevelDB/RocksDB coverage; tuned Gradle test tasks (retries, heap/fork/jvmArgs, Windows excludes); fixed flaky cases (port checks viaServerSocket, documented Trie flakiness).--es,--fast-forward, seed-node flags) in favor of config keys; addedevent.subscribe.enablewith defaults in config; expandedArgsmappings and guardrails; saferNeedBeanConditionnull checks.NativeMessageQueue.stop()now nulls state and resets the singleton; minor template tweaks to mark fields as “Optional”; fixed typos inapi.protocomments.Written for commit da57158. Summary will update on new commits.