Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMultiple test classes removed JUnit parameterization: Changes
Sequence Diagram(s)sequenceDiagram
participant JUnit as JUnit/Runner
participant Param as Parameterized Runner
participant Test as TestClass
rect `#DDEBF7`
Note over JUnit,Param: Old flow (parameterized)
JUnit->>Param: invoke parameterized runner
Param->>Test: instantiate per-parameter (ctor with params)
Test->>Test: setup using provided params
Test->>JUnit: run tests (multiple instances)
end
rect `#F2F2F2`
Note over JUnit,Test: New flow (non-parameterized, randomized)
JUnit->>Test: instantiate once (no-arg constructor)
Test->>Test: constructor initializes random config (Rnd/TestUtils)
Test->>Test: setup uses random config
Test->>JUnit: run tests (single instance)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
|
@CodeRabbit review the test changes and also checked if i regressed anything? |
🧩 Analysis chain🏁 Script executed: Length of output: 1122 🏁 Script executed: Length of output: 265188 @bluestreak01 I've reviewed the test changes in PR #6347. Here's my analysis: Summary of ChangesThe PR converts parameterized tests to use randomization by:
Key Findings✅ No Functional Regressions Detected
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
core/src/test/java/io/questdb/test/griffin/TableBackupTest.java (1)
105-120: Remove unused parameterized test data provider.The
data()method and its@Parameterized.Parametersannotation are no longer used after removing@RunWith(Parameterized.class)from the class. This is dead code that should be removed.Apply this diff to remove the unused method:
- @Parameterized.Parameters(name = "{0}-{1}") - public static Collection<Object[]> data() { - return Arrays.asList(new Object[][]{ - {AbstractCairoTest.WalMode.WITH_WAL, PartitionBy.HOUR}, - {AbstractCairoTest.WalMode.WITH_WAL, PartitionBy.DAY}, - {AbstractCairoTest.WalMode.WITH_WAL, PartitionBy.WEEK}, - {AbstractCairoTest.WalMode.WITH_WAL, PartitionBy.MONTH}, - {AbstractCairoTest.WalMode.WITH_WAL, PartitionBy.YEAR}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.NONE}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.HOUR}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.DAY}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.WEEK}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.MONTH}, - {AbstractCairoTest.WalMode.NO_WAL, PartitionBy.YEAR} - }); - } -Also consider removing the now-unused import at line 62:
-import org.junit.runners.Parameterized;core/src/test/java/io/questdb/test/griffin/AsOfJoinTest.java (5)
835-843: Same trailing comma issue in market_data DDL.The last select item ends with a comma before FROM.
Apply:
@@ - ('2025'::timestamp + x * 100_000L + rnd_int(0, 10_000, 0))::" + rightTableTimestampType.getTypeName() + " as ts, - from long_sequence(10_000) + ('2025'::timestamp + x * 100_000L + rnd_int(0, 10_000, 0))::" + rightTableTimestampType.getTypeName() + " as ts + from long_sequence(10_000)
1754-1763: Likely copy/paste bug: inserting into t1 instead of t2.After creating t2, subsequent inserts target t1, leaving t2 empty and weakening the test.
Change targets to t2:
- execute("INSERT INTO t1 values ('2000-02-07T14:00:00.000000Z', 8, 'a');"); - execute("INSERT INTO t1 values ('2000-02-08T02:00:00.000000Z', 9, 'a');"); - execute("INSERT INTO t1 values ('2000-02-08T02:00:00.000000Z', 10, 'a');"); - execute("INSERT INTO t1 values ('2000-02-08T02:00:00.000000Z', 10, 'c');"); - execute("INSERT INTO t1 values ('2000-02-08T21:00:00.000000Z', 11, 'a');"); - execute("INSERT INTO t1 values ('2000-02-09T15:00:00.000000Z', 12, 'a');"); - execute("INSERT INTO t1 values ('2000-02-09T20:00:00.000000Z', 13, 'a');"); - execute("INSERT INTO t1 values ('2000-02-09T20:00:00.000000Z', 13, 'c');"); - execute("INSERT INTO t1 values ('2000-02-10T16:00:00.000000Z', 14, 'a');"); + execute("INSERT INTO t2 values ('2000-02-07T14:00:00.000000Z', 8, 'a');"); + execute("INSERT INTO t2 values ('2000-02-08T02:00:00.000000Z', 9, 'a');"); + execute("INSERT INTO t2 values ('2000-02-08T02:00:00.000000Z', 10, 'a');"); + execute("INSERT INTO t2 values ('2000-02-08T02:00:00.000000Z', 10, 'c');"); + execute("INSERT INTO t2 values ('2000-02-08T21:00:00.000000Z', 11, 'a');"); + execute("INSERT INTO t2 values ('2000-02-09T15:00:00.000000Z', 12, 'a');"); + execute("INSERT INTO t2 values ('2000-02-09T20:00:00.000000Z', 13, 'a');"); + execute("INSERT INTO t2 values ('2000-02-09T20:00:00.000000Z', 13, 'c');"); + execute("INSERT INTO t2 values ('2000-02-10T16:00:00.000000Z', 14, 'a');");
2901-2905: Hint aliases don’t match the query.Hint uses “orders md” but the query aliases are t1/t2, so the hint is ignored.
Fix:
- query = "SELECT /*+ avoid_lt_binary_search(orders md) */ * FROM t1 LT JOIN t2 TOLERANCE 2s;"; + query = "SELECT /*+ avoid_lt_binary_search(t1 t2) */ * FROM t1 LT JOIN t2 TOLERANCE 2s;";
3664-3669: Hint name likely incorrect; useasof_linear(...)(case-insensitive) instead ofASOF_LINEAR_SEARCH(...).Planner recognizes
asof_linear,asof_index_search,asof_memoized.ASOF_LINEAR_SEARCHmay be ignored.Update:
- final String sql = """ - SELECT /*+ ASOF_LINEAR_SEARCH(m s) */m.sym, ts + final String sql = """ + SELECT /*+ asof_linear(m s) */ m.sym, tsAlso consider renaming the test (it doesn’t “Throw” anymore).
825-833: SQL syntax confirmed: trailing comma before FROM is invalid and must be removed.The trailing comma on line 829 after
as ts,creates invalid SQL when the FROM clause follows directly on the next line. This will cause a parse-time failure. The suggested fix is correct—remove the comma.core/src/test/java/io/questdb/test/griffin/CheckpointTest.java (1)
85-92: Restore deterministic coverage for scoreboard formats.
Switching from the parameterized sweep to a random boolean means SCOREBOARD_FORMAT=2 might never be executed in a run, so regressions in that configuration can slip through and failures become harder to reproduce. Please keep deterministic coverage (e.g., cycle both values or provide an override that the suite drives) while still meeting the speed objective. Based on PR objectives.
🧹 Nitpick comments (8)
core/src/test/java/io/questdb/test/griffin/CopyImportTest.java (1)
55-57: Randomization reduces deterministic coverage but speeds up tests.The constructor now randomly selects
walEnabledinstead of testing both configurations exhaustively. This aligns with the PR objective to speed up tests, but each run will only exercise one WAL configuration.Consider ensuring:
- The seed used by
TestUtils.generateRandom(LOG)is logged for reproducibility- CI optionally runs with deterministic overrides (e.g., system property
-Dqdb.test.wal=true/false) to guarantee both paths are testedcore/src/test/java/io/questdb/test/griffin/AsOfJoinNoKeyTest.java (1)
215-218: Clean refactor: switch statement → switch expression.The conversion to a switch expression is correct and produces more concise code while maintaining identical behavior.
core/src/test/java/io/questdb/test/griffin/TableBackupTest.java (1)
92-103: Random initialization looks good; unreachable default case.The migration from parameterized to random initialization aligns with the PR goal to speed up tests. However, the
defaultcase in the switch statement is unreachable becausernd.nextInt(5)returns values 0-4, all of which are explicitly handled.Consider removing the unreachable default case:
public TableBackupTest() { Rnd rnd = TestUtils.generateRandom(LOG); isWal = rnd.nextBoolean(); switch (rnd.nextInt(5)) { case 0 -> this.partitionBy = PartitionBy.HOUR; case 1 -> this.partitionBy = PartitionBy.MONTH; case 2 -> this.partitionBy = PartitionBy.DAY; case 3 -> this.partitionBy = PartitionBy.WEEK; case 4 -> this.partitionBy = PartitionBy.YEAR; - default -> this.partitionBy = PartitionBy.NONE; + default -> throw new IllegalStateException("Unexpected value: " + rnd.nextInt(5)); } }core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java (2)
169-171: Verify seed logging for reproducibility.The conversion from parameterized to random initialization is syntactically correct. However, this introduces a trade-off: tests with
Assume.assumeTrue(convertToParquet)orAssume.assumeFalse(convertToParquet)(e.g., lines 187, 421, 670, 798, 1042, 1098) will now be randomly skipped ~50% of the time, reducing per-run coverage in exchange for faster execution.Ensure that
TestUtils.generateRandom(LOG)logs the seed or supports seed override (e.g., via system property like-Dqdb.test.seed) so failures can be reproduced with the same random value.Based on PR objectives, consider adding a system property override (e.g.,
-Dqdb.test.parquet=true|false) to allow deterministic selection when needed for debugging or CI matrix runs.
74-160: LGTM: Text block conversion improves readability.The conversion of multi-line string constants to Java text blocks is correct and improves readability. Note that text blocks require Java 15+ as a standard feature.
core/src/test/java/io/questdb/test/griffin/AsOfJoinTest.java (1)
49-53: Make timestamp-type randomization reproducible and overridable.Add system-property overrides and log the chosen combo to aid flakiness triage and CI matrices.
Apply:
@@ - public AsOfJoinTest() { - Rnd rnd = TestUtils.generateRandom(LOG); - this.leftTableTimestampType = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; - this.rightTableTimestampType = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; - } + public AsOfJoinTest() { + Rnd rnd = TestUtils.generateRandom(LOG); + TestTimestampType left = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; + TestTimestampType right = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; + String leftOverride = System.getProperty("qdb.test.leftTsType"); + String rightOverride = System.getProperty("qdb.test.rightTsType"); + this.leftTableTimestampType = leftOverride != null ? TestTimestampType.valueOf(leftOverride.toUpperCase()) : left; + this.rightTableTimestampType = rightOverride != null ? TestTimestampType.valueOf(rightOverride.toUpperCase()) : right; + LOG.info().$("AsOfJoinTest ts types L=") + .$(leftTableTimestampType).$(", R=").$(rightTableTimestampType).$(); + }Optionally document: -Dqdb.test.leftTsType=MICRO|NANO, -Dqdb.test.rightTsType=MICRO|NANO.
Would you like me to add a tiny README blurb listing these knobs?
core/src/test/java/io/questdb/test/griffin/AggregateTest.java (2)
72-74: Reproducible and configurableenableParallelGroupBy.Allow a system-property override and log the chosen mode to ease CI sharding and failure reproduction.
Apply:
@@ - public AggregateTest() { - this.enableParallelGroupBy = TestUtils.generateRandom(LOG).nextBoolean(); - } + public AggregateTest() { + String override = System.getProperty("qdb.test.parallelGroupBy"); + this.enableParallelGroupBy = override != null + ? Boolean.parseBoolean(override) + : TestUtils.generateRandom(LOG).nextBoolean(); + LOG.info().$("AggregateTest parallelGroupBy=").$(enableParallelGroupBy).$(); + }Recommended CI knobs: -Dqdb.test.parallelGroupBy=true|false.
65-74: Seed visibility for timestampType selection.
timestampTypedepends on a static RNG initialised withTestUtils.generateRandom(null). Prefer passingLOGto ensure the seed is emitted like elsewhere.Change:
- private static final Rnd rnd = TestUtils.generateRandom(null); + private static final Rnd rnd = TestUtils.generateRandom(LOG);Please confirm
AbstractCairoTest.LOGis static and available in this context; otherwise we can move RNG init into setUp().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/test/java/io/questdb/test/griffin/AggregateTest.java(47 hunks)core/src/test/java/io/questdb/test/griffin/AlterTableChangeColumnTypeTest.java(11 hunks)core/src/test/java/io/questdb/test/griffin/AlterTableConvertPartitionTest.java(6 hunks)core/src/test/java/io/questdb/test/griffin/AsOfJoinFuzzTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/AsOfJoinNoKeyTest.java(2 hunks)core/src/test/java/io/questdb/test/griffin/AsOfJoinTest.java(2 hunks)core/src/test/java/io/questdb/test/griffin/CheckpointTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/CopyImportTest.java(3 hunks)core/src/test/java/io/questdb/test/griffin/InsertTest.java(48 hunks)core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java(10 hunks)core/src/test/java/io/questdb/test/griffin/ParallelLatestByTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/TableBackupTest.java(2 hunks)core/src/test/java/io/questdb/test/griffin/engine/functions/bool/InLongFunctionFactoryTest.java(8 hunks)core/src/test/java/io/questdb/test/griffin/engine/table/parquet/PartitionEncoderTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-04T09:16:27.366Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 5996
File: core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java:350-356
Timestamp: 2025-08-04T09:16:27.366Z
Learning: In QuestDB ILP tests, maximum dimension values like `(1 << 28) - 1` are intentionally hard-coded instead of using constants like `ArrayView.DIM_MAX_LEN` to serve as "canaries" that will cause test failures if the API limits change, ensuring developers are aware of breaking changes to the ILP API.
Applied to files:
core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java
⏰ 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). (31)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
🔇 Additional comments (19)
core/src/test/java/io/questdb/test/griffin/CopyImportTest.java (2)
185-186: LGTM!The assertions correctly use
TestUtils.assertEqualswith the model's getters. This approach properly handlesCharSequencecomparisons.
1128-1134: LGTM!The assertion updates consistently use
TestUtils.assertEqualswith model getters across all parameter combinations in the loop. The logic is sound.core/src/test/java/io/questdb/test/griffin/AsOfJoinNoKeyTest.java (2)
45-50: Randomization logic is correct and aligns with PR objectives.The constructor correctly initializes test parameters using random selection. This replaces the previous parameterized approach that tested all 8 combinations (2 join types × 2 left timestamp types × 2 right timestamp types) with a single random combination per test run, reducing execution time at the cost of deterministic coverage.
200-200: Note:testSelfJoinnow skips ~50% of test runs.The
Assume.assumeTruecondition means this test only executes whenrightTableTimestampTypeis randomly selected asMICRO. When it'sNANO(50% probability), the test is skipped entirely. This is a departure from the parameterized approach, where this configuration would have been explicitly included or excluded.Ensure this skip rate is acceptable for coverage requirements.
core/src/test/java/io/questdb/test/griffin/engine/functions/bool/InLongFunctionFactoryTest.java (8)
41-45: LGTM! Text block improves readability.The conversion from string concatenation to a text block makes the expected output clearer without changing functionality.
59-59: Excellent performance improvement!Reducing from 100M to 1M rows significantly speeds up test execution while maintaining sufficient coverage for testing the IN clause with bind variables.
74-104: LGTM! Text blocks enhance readability.The conversion to text blocks for both the expected results and query plans (JIT and non-JIT variants) makes these multi-line assertions much more maintainable.
115-115: Consistent performance optimization.The same 100x data reduction applied here ensures this test with 200 bind variables also runs efficiently.
130-149: LGTM! Text blocks improve maintainability.The text block format is especially beneficial here for the long filter list in the query plan assertion, making it easier to read and verify.
159-165: LGTM! Clean text block conversion.
179-182: LGTM! Text block conversion is correct.
196-200: LGTM! Final text block conversion is correct.core/src/test/java/io/questdb/test/griffin/AsOfJoinFuzzTest.java (1)
55-59: LGTM! Conversion to random initialization is sound and reproducible.The constructor correctly initializes timestamp types randomly, replacing the parameterized test approach. This reduces execution time by testing one random combination of
(leftTableTimestampType, rightTableTimestampType)per run instead of all 4 combinations.Coverage trade-off: The test now covers 1 of 4 timestamp-type combinations per run, relying on randomness over time rather than deterministic sweeps. This is acceptable given the PR's speed optimization goal.
Reproducibility: Confirmed.
TestUtils.generateRandom(LOG)logs seeds to both the logger and stdout (e.g.,random seeds: 12345L, 67890L), enabling reproduction of any test failures.core/src/test/java/io/questdb/test/griffin/ParallelLatestByTest.java (1)
58-62: Seed logging confirmed; code change is sound.The
TestUtils.generateRandom(Log log)method logs random seeds to both the logger and stdout, enabling reproducibility of any random test failures. Seeds are derived fromSystem.nanoTime()andSystem.currentTimeMillis()and printed at test runtime, allowing developers to rerun tests with the same seeds when needed.The implementation in
ParallelLatestByTestis correct and reproducible.core/src/test/java/io/questdb/test/griffin/engine/table/parquet/PartitionEncoderTest.java (2)
102-102: Row count reduced for performance.The test now uses 1M rows instead of 10M, a 10x reduction that significantly speeds up execution. This aligns with the PR's goal to avoid resource-heavy tests on CI.
Ensure 1M rows provides sufficient smoke test coverage for the parquet encoder. Consider whether edge cases related to larger datasets (memory pressure, chunking, performance characteristics) are covered elsewhere or if this reduction is acceptable.
45-47: Randomization with reproducibility support; no action required.The constructor randomly selects one
TestTimestampTypeper run instead of testing both MICRO and NANO deterministically, aligning with the PR's goal to speed up tests. Randomization is reproducible:TestUtils.generateRandom(LOG)logs the seed to both the logger and stdout (random seeds: {s0}L, {s1}L), enabling reproduction of failures.core/src/test/java/io/questdb/test/griffin/InsertTest.java (2)
88-91: Text block conversions improve readability.Multi-line string literals throughout the file have been converted to Java text blocks (
"""..."""), improving readability by eliminating string concatenation and escape sequences. These are mechanical refactors with no functional changes to test assertions.Note: Text blocks require Java ≥ 15. Based on learnings from PR objectives, this requirement has been verified with the project's maven-compiler-plugin configuration.
Also applies to: 94-98, 101-106, 116-119, 122-126, 129-134, 447-450, 457-460, 467-470, 477-480, 487-490, 495-498, 503-506, 511-514, 549-552, 557-560, 565-568, 573-576, 604-608, 635-639, 701-704, 715-718, 729-732, 743-746, 754-757, 764-767, 774-777, 784-787, 794-797, 804-807, 814-817, 824-827, 834-837, 844-847, 854-857, 864-867, 874-877, 889-892, 899-902, 914-917, 924-927, 939-942, 949-952, 964-967, 974-977, 984-987, 997-1000, 1011-1014, 1072-1076, 1098-1102, 1113-1117, 1177-1181, 1218-1221, 1286-1298, 1300-1309, 1317-1320, 1339-1344, 1348-1351, 1362-1366, 1379-1383, 1396-1400, 1412-1415, 1425-1428, 1440-1443, 1453-1456, 1469-1473, 1486-1490, 1503-1507, 1526-1529, 1541-1544, 1564-1567, 1586-1589, 1592-1595, 1605-1608, 1636-1639, 1658-1661, 1664-1667, 1684-1689, 1693-1696, 1726-1732, 1735-1738, 1760-1772, 1812-1821, 1830-1840, 1845-1855, 2062-2067, 2087-2092
63-65: The web search didn't provide the implementation details. Let me verify by checking the codebase directly with a targeted search for the actual method.Based on my analysis of the search results:
Finding seeds are retrievable but logging verification needed
The search results show that developers can use the same seed values to achieve deterministic pseudo-random behavior, and the codebase extensively uses
getSeed0()andgetSeed1()methods to retrieve seeds for reproduction. However, I could not conclusively verify from the available output whetherTestUtils.generateRandom(LOG)automatically logs these seeds for easy reproduction.The WalWriterFuzzTest comment at lines 48-51 suggests there should be a logging line documenting seeds, but the actual logging implementation was not visible in the search results.
Conclusion:
The original review comment's concern about seed logging for reproducibility is valid but unverified. The issue identified requires manual verification:
Ensure random seed logging for test reproducibility
The random initialization in the constructor relies on
TestUtils.generateRandom(LOG)to provide reproducible seeding via seed logging. Verify that:
TestUtils.generateRandom(LOG)logs the two seeds to standard output/logs- Test failure output includes seed values for developers to reproduce the exact scenario
- If seeds aren't automatically logged, add explicit logging:
LOG.info().$("walEnabled seed: ").$(seed0).$(" ").$(seed1).$();core/src/test/java/io/questdb/test/griffin/AggregateTest.java (1)
134-139: No issues detected—the code is correct as written.The method
replaceTimestampSuffix1(String expected, String timestampType)is publicly defined inAbstractCairoTest.java(line 565) and the usage at line 134 matches its signature exactly. The method is available to all test classes that extendAbstractCairoTest, which is the standard pattern for test inheritance in this codebase. Similar usage appears across multiple test files (WindowFunctionTest, IntervalFilterTest, O3Test, and others), confirming the method is properly accessible.
core/src/test/java/io/questdb/test/griffin/AlterTableChangeColumnTypeTest.java
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/griffin/AlterTableConvertPartitionTest.java
Show resolved
Hide resolved
|
@CodeRabbit i addressed your comments, please check |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 191 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 116 🏁 Script executed: Length of output: 638 🏁 Script executed: Length of output: 58859 🏁 Script executed: Length of output: 6071 🏁 Script executed: Length of output: 102 🏁 Script executed: Length of output: 792 🏁 Script executed: Length of output: 503 🏁 Script executed: Length of output: 16164 @bluestreak01 I've reviewed your changes in commit What you addressed ✅Good cleanup work:
Seed logging is already handled: What wasn't addressed
|
|
@CodeRabbit we run CI very often, random seeds are logged and we will eventually go over all combinations. CI boxes are extremely slow and resource constrained, running full suite of tests would either max out our parallelism or box capacity. We are squeezed from both sides. Review my new changes, i applied the same pattern to other tests |
🧩 Analysis chain🏁 Script executed: Length of output: 326 🏁 Script executed: Length of output: 3700 🏁 Script executed: Length of output: 1029 🏁 Script executed: Length of output: 266275 🏁 Script executed: Length of output: 48 🏁 Script executed: Length of output: 8661 🏁 Script executed: Length of output: 19636 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 6872 🏁 Script executed: Length of output: 688 🏁 Script executed: Length of output: 24590 🏁 Script executed: Length of output: 10423 🏁 Script executed: Length of output: 7704 🏁 Script executed: Length of output: 2848 🏁 Script executed: Length of output: 5788 @bluestreak01 Thanks for the update. I pulled the latest (HEAD 2584a18 “speedup cairo & fuzz tests”) and reviewed the new conversions. Highlights and checks: What you changed (and looks good)
Sanity checks I ran
Risks and small nits
Verdict
If you want me to scan any other test group in this PR, ping me the paths and I’ll take a pass. ✅ Actions performedReview triggered.
|
bluestreak01
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/src/test/java/io/questdb/test/cairo/fuzz/DedupInsertFuzzTest.java (1)
186-190: Potential test failure: Assumption removed but TODO remains.The
Assume.assumeFalse(convertToParquet)guard was removed on line 189, but the TODO comment on line 188 states: "Enable this test when adding columns after Parquet conversion is supported." SinceconvertToParquetis now randomlytruein some runs (line 80), this test may fail when that condition occurs, despite the feature not being supported yet.Consider one of these approaches:
- Restore the assumption if the feature is still unsupported:
// TODO(eugene): Enable this test when adding columns after Parquet conversion is supported - Assume.assumeFalse(convertToParquet); + Assume.assumeFalse(convertToParquet);
- Or remove the TODO and verify the test passes with Parquet conversion enabled.
core/src/test/java/io/questdb/test/cairo/fuzz/CheckpointFuzzTest.java (1)
55-62: Problematic: Static lifecycle methods called from instance constructor.The constructor calls
tearDownStatic()andsetUpStatic()(which are@AfterClassand@BeforeClassmethods) when the randomly selectedscoreboardFormatdiffers from the staticSCOREBOARD_FORMAT. This pattern is problematic because:
- JUnit may create multiple test instances: JUnit can instantiate the test class multiple times, and each instantiation could trigger these static lifecycle methods unpredictably.
- Race conditions: If tests run in parallel, this could cause resource conflicts.
- Lifecycle violations:
@BeforeClass/@AfterClassmethods should run once per test class, not per instance.- Constructor side effects: Constructors should be lightweight and not perform complex setup/teardown.
Consider these alternatives:
- Use JUnit 5's
@ParameterizedTestwith explicit parameters instead of randomization- Use a custom JUnit runner that properly handles the scoreboard format setup
- Create separate test classes for each scoreboard format
- Use a
@BeforeClassmethod to randomize and set up once, storing the choice in a static fieldIf you must keep this pattern, at minimum add synchronization and document the approach:
public CheckpointFuzzTest() throws Exception { + // Note: This pattern is non-standard and may cause issues with parallel test execution + synchronized (CheckpointFuzzTest.class) { int scoreboardFormat = TestUtils.generateRandom(LOG).nextBoolean() ? 1 : 2; if (scoreboardFormat != SCOREBOARD_FORMAT) { SCOREBOARD_FORMAT = scoreboardFormat; tearDownStatic(); setUpStatic(); } + } }core/src/test/java/io/questdb/test/cairo/o3/O3MetricsTest.java (1)
41-43: Doc nit: incorrect MILLENNIUM comment946684800000000µs is 2000-01-01T00:00:00Z, not 2020-01-01.
- private static final long MILLENNIUM = 946684800000000L; // 2020-01-01T00:00:00 + private static final long MILLENNIUM = 946_684_800_000_000L; // 2000-01-01T00:00:00Z
♻️ Duplicate comments (1)
core/src/test/java/io/questdb/test/griffin/AlterTableChangeColumnTypeTest.java (1)
63-70: Still skippingassumeNonWalpaths most runs.This ctor only yields
!walEnabled && partitionedabout 25 % of the time, so every test guarded byassumeNonWal()will silently skip in most CI runs. Previously the parameterisation guaranteed that combo (and the others) on every execution. Please restore deterministic coverage—iterate the oldModeset or wire fixed overrides—before merging. Same concern I raised earlier.
🧹 Nitpick comments (10)
core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpSenderTest.java (1)
91-95: LGTM! Constructor correctly implements randomized test configuration.The conversion from parameterized to randomized testing is implemented correctly. The use of
TestUtils.generateRandom(AbstractCairoTest.LOG)ensures seed logging for reproducibility.Optional enhancement: Consider adding system property overrides to allow forcing specific configurations when needed, as discussed in the PR review:
public LineTcpSenderTest() { Rnd rnd = TestUtils.generateRandom(AbstractCairoTest.LOG); this.walEnabled = System.getProperty("qdb.test.wal") != null ? Boolean.parseBoolean(System.getProperty("qdb.test.wal")) : rnd.nextBoolean(); this.timestampType = System.getProperty("qdb.test.tsType") != null ? TestTimestampType.valueOf(System.getProperty("qdb.test.tsType")) : (rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO); }This would allow developers and CI to deterministically test specific combinations (e.g.,
-Dqdb.test.wal=true -Dqdb.test.tsType=NANO) while keeping randomization as the default.core/src/test/java/io/questdb/test/cutlass/line/tcp/AlterTableLineTcpReceiverTest.java (1)
76-78: LGTM – randomized initialization correctly implemented.The constructor properly uses
TestUtils.generateRandom(AbstractCairoTest.LOG)for seeded randomization of the timestamp type. The seed logging enables reproducibility when a test fails with a specific random configuration.Optional: Log the chosen configuration for easier debugging.
Consider logging the selected
timestampTypevalue to help diagnose test failures without needing to re-run with the seed:public AlterTableLineTcpReceiverTest() { this.timestampType = TestUtils.generateRandom(AbstractCairoTest.LOG).nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; + LOG.info().$("Test configuration: timestampType=").$(timestampType).$(); }core/src/test/java/io/questdb/test/griffin/TableBackupTest.java (1)
88-109: LGTM – randomization logic correctly handles WAL constraints.The constructor properly excludes
PartitionBy.NONEfor WAL mode (lines 101-107) since WAL requires partitioning, while allowing it for non-WAL mode (line 98). The seed is logged viaTestUtils.generateRandom(LOG)for reproducibility.Optional: Log the chosen test configuration.
To aid debugging without re-running with a specific seed, consider logging the selected configuration:
default -> this.partitionBy = PartitionBy.YEAR; } } +LOG.info().$("Test configuration: isWal=").$(isWal).$(" partitionBy=").$(PartitionBy.toString(partitionBy)).$();core/src/test/java/io/questdb/test/cairo/o3/O3FailureTest.java (1)
1207-1711: Huge expected snapshot retained as literal — optional tidy.Optional: consider externalizing this golden snapshot (resource file) or generating it with a helper to reduce file size/noise. Not required for this PR.
core/src/test/java/io/questdb/test/cutlass/pgwire/PreparedStatementInvalidationTest.java (1)
61-63: Add a system‑property override for WAL randomness.Random is fine for speed, but allow forcing behavior in CI/local repro. Suggested ctor change:
public PreparedStatementInvalidationTest() { - this.walEnabled = TestUtils.generateRandom(LOG).nextBoolean(); + final String override = System.getProperty("qdb.test.wal"); + this.walEnabled = override != null + ? Boolean.parseBoolean(override) + : TestUtils.generateRandom(LOG).nextBoolean(); }Then you can run with: -Dqdb.test.wal=true|false.
core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverTest.java (2)
105-109: Add deterministic knobs for WAL/timestamp type via syspropsEnable reproducible runs and CI splits: honor -Dqdb.test.wal and -Dqdb.test.tsType before falling back to randomness.
Apply:
- public LineTcpReceiverTest() { - Rnd rnd = TestUtils.generateRandom(AbstractCairoTest.LOG); - this.walEnabled = rnd.nextBoolean(); - this.timestampType = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; - } + public LineTcpReceiverTest() { + Rnd rnd = TestUtils.generateRandom(AbstractCairoTest.LOG); + String walProp = System.getProperty("qdb.test.wal"); + this.walEnabled = walProp != null ? Boolean.parseBoolean(walProp) : rnd.nextBoolean(); + String tsProp = System.getProperty("qdb.test.tsType"); + this.timestampType = tsProp != null + ? TestTimestampType.valueOf(tsProp) + : (rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO); + }
240-241: Reset pool listener; prefer null over no-opUse try/finally to reset the engine pool listener and set it back to null to avoid cross-test leakage and overhead.
- engine.setPoolListener((factoryType, thread, name, event, segment, position) -> { - }); + engine.setPoolListener(null);Consider a small helper that scopes listener registration with try/finally.
core/src/test/java/io/questdb/test/cairo/o3/AbstractO3Test.java (1)
94-97: Allow forcing timestamp type via sysprop; keep randomness as fallbackWire -Dqdb.test.tsType (MICRO|NANO) for determinism while preserving current random default.
- public AbstractO3Test() { - this.rnd = TestUtils.generateRandom(LOG); - this.timestampType = rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; - } + public AbstractO3Test() { + this.rnd = TestUtils.generateRandom(LOG); + String tsProp = System.getProperty("qdb.test.tsType"); + this.timestampType = tsProp != null + ? TestTimestampType.valueOf(tsProp) + : (rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO); + }Optionally seed SharedRandom with this rnd for consistent data RNG across runs.
core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java (1)
169-171: Expose Parquet toggle via syspropLet CI/devs force Parquet conversion with -Dqdb.test.parquet; keep random as fallback.
- public ParallelFilterTest() { - this.convertToParquet = TestUtils.generateRandom(LOG).nextBoolean(); - } + public ParallelFilterTest() { + Rnd rnd = TestUtils.generateRandom(LOG); + String prop = System.getProperty("qdb.test.parquet"); + this.convertToParquet = prop != null ? Boolean.parseBoolean(prop) : rnd.nextBoolean(); + }core/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.java (1)
53-55: Make timestamp type selectable via syspropSupport -Dqdb.test.tsType (MICRO|NANO) to reproduce runs; fall back to randomness.
- public O3SquashPartitionTest() { - this.timestampType = TestUtils.generateRandom(LOG).nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO; - } + public O3SquashPartitionTest() { + Rnd rnd = TestUtils.generateRandom(LOG); + String tsProp = System.getProperty("qdb.test.tsType"); + this.timestampType = tsProp != null + ? TestTimestampType.valueOf(tsProp) + : (rnd.nextBoolean() ? TestTimestampType.MICRO : TestTimestampType.NANO); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
core/src/test/java/io/questdb/test/cairo/fuzz/CheckpointFuzzTest.java(1 hunks)core/src/test/java/io/questdb/test/cairo/fuzz/DedupInsertFuzzTest.java(1 hunks)core/src/test/java/io/questdb/test/cairo/fuzz/O3MaxLagFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cairo/fuzz/O3ParquetPartitionFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cairo/o3/AbstractO3Test.java(2 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3FailureFuzzTest.java(1 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3FailureTest.java(10 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3MaxLagTest.java(3 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3MetricsTest.java(2 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3SplitPartitionTest.java(8 hunks)core/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.java(24 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/AbstractLineTcpReceiverFuzzTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/AlterTableLineTcpReceiverTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/AlterWalTableLineTcpReceiverTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/BaseLineTcpInsertGeoHashTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpCommitFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpConnectionContextTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpInsertByteGeoHashTest.java(2 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpInsertIntGeoHashTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpInsertLongGeoHashTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpInsertShortGeoHashTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpO3Test.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverDropTableFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverTest.java(27 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverUpdateFuzzTest.java(0 hunks)core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpSenderTest.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/pgwire/PGArraysTest.java(19 hunks)core/src/test/java/io/questdb/test/cutlass/pgwire/PreparedStatementInvalidationTest.java(15 hunks)core/src/test/java/io/questdb/test/griffin/AlterTableChangeColumnTypeTest.java(13 hunks)core/src/test/java/io/questdb/test/griffin/AsOfJoinTest.java(6 hunks)core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java(10 hunks)core/src/test/java/io/questdb/test/griffin/TableBackupTest.java(2 hunks)
💤 Files with no reviewable changes (6)
- core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverFuzzTest.java
- core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverUpdateFuzzTest.java
- core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpCommitFuzzTest.java
- core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverDropTableFuzzTest.java
- core/src/test/java/io/questdb/test/cairo/fuzz/O3MaxLagFuzzTest.java
- core/src/test/java/io/questdb/test/cairo/fuzz/O3ParquetPartitionFuzzTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-04T09:16:27.366Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 5996
File: core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java:350-356
Timestamp: 2025-08-04T09:16:27.366Z
Learning: In QuestDB ILP tests, maximum dimension values like `(1 << 28) - 1` are intentionally hard-coded instead of using constants like `ArrayView.DIM_MAX_LEN` to serve as "canaries" that will cause test failures if the API limits change, ensuring developers are aware of breaking changes to the ILP API.
Applied to files:
core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java
⏰ 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). (30)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (21)
core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpInsertLongGeoHashTest.java (1)
33-161: LGTM! Text block conversions improve readability.The conversion of string concatenations to Java text blocks across all test methods is correct and improves maintainability. The test data and assertions remain functionally unchanged—only the representation has been modernized.
core/src/test/java/io/questdb/test/cairo/o3/O3MaxLagTest.java (1)
1412-1417: Text block conversions improve readability.The conversion from concatenated strings to Java text blocks is a nice improvement. The formatting is correct, with proper tab separators and trailing newlines. The
replaceTimestampSuffixwrapper correctly handles timestamp type variations.Note: Text blocks require Java 15+. Based on the PR discussion, this should already be verified in the project's maven-compiler-plugin configuration.
Also applies to: 1426-1431, 1443-1448
core/src/test/java/io/questdb/test/griffin/AsOfJoinTest.java (4)
49-52: LGTM: Constructor converted to randomized pattern.The parameterized constructor has been replaced with a no-arg constructor that randomly selects timestamp types, following the pattern described in the PR objectives.
TestUtils.generateRandom(LOG)logs seeds for reproducibility.Coverage trade-off: This moves from deterministic full-coverage (all MICRO×NANO combinations every run) to single random pick per run. Consider the PR's recommended mitigations if test flakiness emerges:
- System property overrides (e.g.,
-Dqdb.test.tsType) to force specific combinations- Lightweight CI matrix to cover critical combinations deterministically
1754-1762: Bug fix: Corrected INSERT target table.The test was incorrectly inserting t2's data into t1, leaving t2 empty. Since
testInterleaved2is designed to test interleaving between two tables andassertResultSetsMatch("t1", "t2")at Line 1764 requires both tables to have data, this change correctly populates t2 instead of duplicating data in t1.
2901-2901: LGTM: Hint added to test disabling fast path.The
avoid_lt_binary_search(t1 t2)hint forces the query to use the full-fat join path instead of the optimized binary search. The assertion at Line 2908 correctly verifies the plan shows "Lt Join" without "Fast Scan".
3664-3707: LGTM: Added verification that dynamic symbols skip single-symbol optimization.The
ASOF_LINEAR(m s)hint forces a linear scan, and the new assertions (Lines 3690-3707) correctly verify that when the slave table usescast(sym_str as symbol)(Line 3667), the query plan shows "AsOf Join Light" instead of "AsOf Join Single Symbol". This confirms that dynamically cast symbols properly bypass the single-symbol optimization.core/src/test/java/io/questdb/test/cutlass/line/tcp/AlterWalTableLineTcpReceiverTest.java (1)
77-79: LGTM - Randomized timestamp type initialization.The constructor correctly initializes
timestampTypewith a random choice between MICRO and NANO, aligning with the PR's goal of replacing parameterized tests with randomized execution.core/src/test/java/io/questdb/test/cairo/fuzz/DedupInsertFuzzTest.java (1)
79-81: LGTM - Randomized Parquet conversion flag.The constructor correctly randomizes
convertToParquetinstead of using parameterized test execution.core/src/test/java/io/questdb/test/cutlass/pgwire/PGArraysTest.java (1)
55-57: LGTM - Randomized WAL enablement.The constructor correctly randomizes
walEnabledusing the same RNG instance (bufferSizeRnd) that was already being used for other random test parameters.core/src/test/java/io/questdb/test/cairo/o3/O3FailureFuzzTest.java (1)
89-92: No issues found. The initialization order is safe and correct.The parent class
AbstractO3Testinitializes therndfield immediately in its constructor (line 95:this.rnd = TestUtils.generateRandom(LOG);). Since Java executes the parent constructor fully before the child constructor body continues,super.rndis guaranteed to be initialized whenO3FailureFuzzTest()accesses it on line 91. NoNullPointerExceptionrisk exists.core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpO3Test.java (1)
70-72: LGTM – straightforward randomization with proper seed logging.The constructor correctly uses
TestUtils.generateRandom(LOG)to randomly initializewalEnabledwith reproducible seed logging.core/src/test/java/io/questdb/test/cutlass/line/tcp/BaseLineTcpInsertGeoHashTest.java (1)
46-49: LGTM – proper randomization with seeded RNG.The constructor correctly initializes
walEnabledusing a seeded random generator fromTestUtils.generateRandom(AbstractCairoTest.LOG), ensuring reproducibility.core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpConnectionContextTest.java (1)
61-65: LGTM – correctly randomizes multiple test dimensions.The constructor properly randomizes both
walEnabledandtimestampTypeusing a seeded random generator, maintaining reproducibility through seed logging.core/src/test/java/io/questdb/test/cutlass/line/tcp/AbstractLineTcpReceiverFuzzTest.java (1)
104-106: LGTM – randomization correctly implemented for fuzz testing.The constructor properly uses
TestUtils.generateRandom(LOG)to randomly initializewalEnabledwith seed logging for reproducibility. This is appropriate for fuzz testing scenarios.core/src/test/java/io/questdb/test/cutlass/pgwire/PreparedStatementInvalidationTest.java (1)
110-113: Text blocks for expected results look good; verify JDK target.Approving the conversions; they improve clarity with no behavior change. As with other files, ensure build targets Java ≥ 15 (preferably 17).
Use the same maven-compiler verification as suggested in the other comment.
Also applies to: 158-161, 221-224, 255-259, 481-489, 528-531, 659-670, 705-716, 751-762, 797-808
core/src/test/java/io/questdb/test/cairo/o3/O3FailureTest.java (1)
663-670: Text blocks are fully compatible; Java 17 supports them.Verification confirms the core module targets Java 17 via
<javac.target>17</javac.target>in core/pom.xml, which exceeds the Java 15 minimum required for text blocks. All text-block replacements throughout the file (lines 663–670, 680–687, 1756–1760, 2818–2825, 2880–2887, 3713–3716, 3726–3730) compile safely.core/src/test/java/io/questdb/test/cutlass/line/tcp/LineTcpReceiverTest.java (2)
1028-1033: Bulk text-block conversions in assertionsNice cleanup; expected layouts match prior semantics. No issues spotted.
Also applies to: 1710-1714, 1739-1743, 1810-1813, 1836-1839
457-467: Verified: Java release 17 confirmed in core module build configurationThe core module is configured to target Java 17 via the property chain:
javac.target=17(core/pom.xml:35) →javac.compile.source/target=${javac.target}(core/pom.xml:629-630) → maven-compiler-plugin source/target configuration (core/pom.xml:91-92). This meets the requirement for text blocks and all string escape sequences used in the conversion.core/src/test/java/io/questdb/test/cairo/o3/O3MetricsTest.java (1)
61-73: Text-block assertions: looks correctExpected outputs preserved; suffix handling via replaceTimestampSuffix remains intact.
Also applies to: 100-106
core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java (1)
74-141: Text-block conversions across expected datasetsReadability improvement; values align with prior expectations. No functional changes detected.
Also applies to: 271-275, 516-525, 611-620, 930-936, 974-985, 1023-1030, 1177-1189
core/src/test/java/io/questdb/test/cairo/o3/O3SquashPartitionTest.java (1)
112-116: Text-block assertions: good migrationBlocks match prior concatenations; improves maintainability.
Also applies to: 183-187, 200-205, 217-221, 233-237, 249-253, 305-308, 357-363, 404-414
also: more descriptive logging
io.questdb.test.cairo.o3.O3FailureFuzzTest Appears in 2 logs: ./logs/cairo-2, ./logs/fuzz2 io.questdb.test.cairo.mv.MatViewFuzzTest Appears in 2 logs: ./logs/cairo-2, ./logs/fuzz2 io.questdb.test.cutlass.http.line.LineHttpReceiverFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-2 io.questdb.test.cutlass.line.tcp.LineTcpReceiverFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.cutlass.line.tcp.LineTcpReceiverUpdateFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.std.AtomicIntListFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.cutlass.http.line.HttpSenderMemoryPressureFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-2 io.questdb.test.std.VectFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.cutlass.http.line.LineRawHttpFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-2 io.questdb.test.mp.ConcurrentPoolFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.cutlass.line.tcp.LineTcpCommitFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.mp.ConcurrentQueueFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.cutlass.line.tcp.LineTcpReceiverDropTableFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.std.ParseNanosFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1 io.questdb.test.std.FilesCacheFuzzTest Appears in 2 logs: ./logs/fuzz2, ./logs/other-1
windows ci: 1 4m 9.13s io.questdb.test.cairo.o3.O3Test.testBenchContended 2 2m 57.82s io.questdb.test.cairo.o3.O3Test.testBench 3 2m 56.89s io.questdb.test.cairo.o3.O3Test.testBenchParallel now we are executing just one of these scenario on each run
… into vi_griffin_speedup
[PR Coverage check]😍 pass : 0 / 0 (0%) |
3 participants
Speedup tests by replacing JUnit parameterisation with randomisation.
Some tests were also creating 100M row tables (on CI, because why not)