Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 merged PR #8168: URL: https://github.com/apache/hbase/pull/8168 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 merged PR #8169: URL: https://github.com/apache/hbase/pull/8169 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 merged PR #8167: URL: https://github.com/apache/hbase/pull/8167 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Copilot commented on code in PR #8168:
URL: https://github.com/apache/hbase/pull/8168#discussion_r3162634962
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/TestCompressedWAL.java:
##
@@ -18,18 +18,20 @@
package org.apache.hadoop.hbase.wal;
import java.util.stream.Stream;
-import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedClass;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
Review Comment:
Same issue as the other WAL test: `@ParameterizedClass` + class-level
`@MethodSource` are not standard JUnit Jupiter APIs for parameterization. If
this annotation is not provided by a project-specific dependency, the test will
not compile; if it is ignored, the tests will no longer run per-provider as
expected (especially after converting the base tests from `@TestTemplate` to
`@Test`). Consider moving parameterization to the test methods
(`@ParameterizedTest`) or restoring a `@TestTemplate` setup that is known to
work with HBase’s parameter sets.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/TestCompressedWAL.java:
##
@@ -18,18 +18,20 @@
package org.apache.hadoop.hbase.wal;
import java.util.stream.Stream;
-import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedClass;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
@Tag(RegionServerTests.TAG)
@Tag(MediumTests.TAG)
-@HBaseParameterizedTestTemplate(name = "{index}: provider={0}")
+@ParameterizedClass(name = "{index}: provider={0}")
+@MethodSource("parameters")
Review Comment:
Same issue as the other WAL test: `@ParameterizedClass` + class-level
`@MethodSource` are not standard JUnit Jupiter APIs for parameterization. If
this annotation is not provided by a project-specific dependency, the test will
not compile; if it is ignored, the tests will no longer run per-provider as
expected (especially after converting the base tests from `@TestTemplate` to
`@Test`). Consider moving parameterization to the test methods
(`@ParameterizedTest`) or restoring a `@TestTemplate` setup that is known to
work with HBase’s parameter sets.
##
hbase-compression/hbase-compression-zstd/src/test/java/org/apache/hadoop/hbase/io/compress/zstd/TestHFileCompressionZstd.java:
##
@@ -86,7 +81,6 @@ public void testReconfLevels() throws Exception {
long len_2 = FS.getFileStatus(path_2).getLen();
LOG.info("Level 1 len {}", len_1);
LOG.info("Level 22 len {}", len_2);
-assertTrue("Reconfiguraton with ZSTD_LEVEL_KEY did not seem to work",
len_1 > len_2);
+assertTrue(len_1 > len_2, "Reconfiguraton with ZSTD_LEVEL_KEY did not seem
to work");
Review Comment:
Fix typo in assertion message: 'Reconfiguraton' → 'Reconfiguration'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Copilot commented on code in PR #8169:
URL: https://github.com/apache/hbase/pull/8169#discussion_r3162630470
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/TestCompressedWAL.java:
##
@@ -18,18 +18,20 @@
package org.apache.hadoop.hbase.wal;
import java.util.stream.Stream;
-import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedClass;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
Review Comment:
Same issue as in `TestCompressedWALValueCompression`: JUnit Jupiter doesn't
support this class-level parameterization API in its standard params module. To
keep the test running per provider value, use a supported mechanism (retain the
prior HBase parameterized test template, or refactor to method-level
`@ParameterizedTest` + `@MethodSource`).
##
hbase-compression/hbase-compression-zstd/src/test/java/org/apache/hadoop/hbase/io/compress/zstd/TestHFileCompressionZstd.java:
##
@@ -86,7 +81,6 @@ public void testReconfLevels() throws Exception {
long len_2 = FS.getFileStatus(path_2).getLen();
LOG.info("Level 1 len {}", len_1);
LOG.info("Level 22 len {}", len_2);
-assertTrue("Reconfiguraton with ZSTD_LEVEL_KEY did not seem to work",
len_1 > len_2);
+assertTrue(len_1 > len_2, "Reconfiguraton with ZSTD_LEVEL_KEY did not seem
to work");
Review Comment:
Fix typo in assertion message: 'Reconfiguraton' should be 'Reconfiguration'.
```suggestion
assertTrue(len_1 > len_2, "Reconfiguration with ZSTD_LEVEL_KEY did not
seem to work");
```
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/CompressedWALTestBase.java:
##
@@ -71,12 +68,12 @@ public abstract class CompressedWALTestBase {
Arrays.fill(VALUE, off, (off += 1597), (byte) 'Q');
}
- @TestTemplate
+ @Test
public void test() throws Exception {
testForSize(1000);
}
- @TestTemplate
+ @Test
public void testLarge() throws Exception {
testForSize(1024 * 1024);
}
Review Comment:
These methods were previously `@TestTemplate`, which allows repeated
execution under a template/parameterization extension. Converting them to plain
`@Test` will run each only once per subclass instance and will not
automatically iterate over compression/provider parameters. If the intent is to
execute `test()`/`testLarge()` for each parameter set, revert these back to
`@TestTemplate` (with the appropriate template extension/annotation) or move to
supported JUnit parameterized methods (method-level `@ParameterizedTest`) so
the parameter matrix is preserved.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/TestCompressedWAL.java:
##
@@ -18,18 +18,20 @@
package org.apache.hadoop.hbase.wal;
import java.util.stream.Stream;
-import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedClass;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
@Tag(RegionServerTests.TAG)
@Tag(MediumTests.TAG)
-@HBaseParameterizedTestTemplate(name = "{index}: provider={0}")
+@ParameterizedClass(name = "{index}: provider={0}")
+@MethodSource("parameters")
Review Comment:
Same issue as in `TestCompressedWALValueCompression`: JUnit Jupiter doesn't
support this class-level parameterization API in its standard params module. To
keep the test running per provider value, use a supported mechanism (retain the
prior HBase parameterized test template, or refactor to method-level
`@ParameterizedTest` + `@MethodSource`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Copilot commented on code in PR #8167:
URL: https://github.com/apache/hbase/pull/8167#discussion_r3162619096
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/TestCompressedWAL.java:
##
@@ -18,18 +18,20 @@
package org.apache.hadoop.hbase.wal;
import java.util.stream.Stream;
-import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedClass;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
@Tag(RegionServerTests.TAG)
@Tag(MediumTests.TAG)
-@HBaseParameterizedTestTemplate(name = "{index}: provider={0}")
+@ParameterizedClass(name = "{index}: provider={0}")
+@MethodSource("parameters")
public class TestCompressedWAL extends CompressedWALTestBase {
Review Comment:
`org.junit.jupiter.params.ParameterizedClass` and a type-level
`@MethodSource` are not part of standard JUnit Jupiter parameterized tests (as
of JUnit 5.10.x/5.11.x), and `@MethodSource` is normally only applicable to
parameterized *methods*. This is likely to fail compilation or silently not
parameterize execution. Consider converting to method-level
`@ParameterizedTest` + `@MethodSource` (passing the parameter into each test
method), or keep using `@TestTemplate` with the existing HBase parameterized
template/extension if class-level parameterization is required.
##
hbase-compression/hbase-compression-zstd/src/test/java/org/apache/hadoop/hbase/io/compress/zstd/TestHFileCompressionZstd.java:
##
@@ -86,7 +81,6 @@ public void testReconfLevels() throws Exception {
long len_2 = FS.getFileStatus(path_2).getLen();
LOG.info("Level 1 len {}", len_1);
LOG.info("Level 22 len {}", len_2);
-assertTrue("Reconfiguraton with ZSTD_LEVEL_KEY did not seem to work",
len_1 > len_2);
+assertTrue(len_1 > len_2, "Reconfiguraton with ZSTD_LEVEL_KEY did not seem
to work");
Review Comment:
Typo in assertion message: `Reconfiguraton` should be `Reconfiguration`.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/CompressedWALTestBase.java:
##
@@ -71,12 +68,12 @@ public abstract class CompressedWALTestBase {
Arrays.fill(VALUE, off, (off += 1597), (byte) 'Q');
}
- @TestTemplate
+ @Test
public void test() throws Exception {
testForSize(1000);
}
- @TestTemplate
+ @Test
public void testLarge() throws Exception {
testForSize(1024 * 1024);
}
Review Comment:
Replacing `@TestTemplate` with `@Test` changes execution semantics: these
methods will run once per test class instance instead of once per template
invocation. If the intended behavior is to execute these tests once per
compression/provider parameter set, this will likely reduce coverage (or break
the parameterization model). A more robust approach is to either (1) keep
`@TestTemplate` and register the corresponding invocation context
provider/extension, or (2) implement method-level `@ParameterizedTest` methods
that accept the parameter and then call `testForSize(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 merged PR #8072: URL: https://github.com/apache/hbase/pull/8072 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 commented on PR #8072: URL: https://github.com/apache/hbase/pull/8072#issuecomment-4332542397 Now the general check looks good. https://github.com/apache/hbase/actions/runs/25016408688/job/73265250014?pr=8072 https://github.com/user-attachments/assets/e55801cd-71a5-454e-891c-af3607b56cdd"; /> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Copilot commented on code in PR #8072:
URL: https://github.com/apache/hbase/pull/8072#discussion_r3149598273
##
hbase-server/src/test/java/org/apache/hadoop/hbase/io/compress/HFileTestBase.java:
##
@@ -82,38 +82,29 @@ public void doTest(Configuration conf, Path path,
Compression.Algorithm compress
// read it back in
LOG.info("Reading with " + fileContext);
int i = 0;
-HFileScanner scanner = null;
-HFile.Reader reader = HFile.createReader(FS, path, cacheConf, true, conf);
-try {
- scanner = reader.getScanner(conf, false, false);
- assertTrue("Initial seekTo failed", scanner.seekTo());
+try (HFile.Reader reader = HFile.createReader(FS, path, cacheConf, true,
conf);
+ HFileScanner scanner = reader.getScanner(conf, false, false)) {
+ assertTrue(scanner.seekTo(), "Initial seekTo failed");
do {
ExtendedCell kv = scanner.getCell();
-assertTrue("Read back an unexpected or invalid KV",
- testKvs.contains(KeyValueUtil.ensureKeyValue(kv)));
+assertTrue(testKvs.contains(KeyValueUtil.ensureKeyValue(kv)),
+ "Read back an unexpected or invalid KV");
i++;
} while (scanner.next());
-} finally {
- reader.close();
- scanner.close();
}
-assertEquals("Did not read back as many KVs as written", i,
testKvs.size());
+assertEquals(i, testKvs.size(), "Did not read back as many KVs as
written");
Review Comment:
`assertEquals` in JUnit Jupiter expects arguments in the order (expected,
actual, message). Here it is currently `assertEquals(i, testKvs.size(), ...)`,
which will invert expected/actual in failure output. Swap the first two
arguments so the expected value is `testKvs.size()` and the actual value is `i`.
##
hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseParameterizedTemplateProvider.java:
##
@@ -40,9 +41,11 @@
*
* When you want to use this provider, annotation the test class with
* {@link HBaseParameterizedTestTemplate}, and provide a static method named
"parameters" for
- * providing the arguments. The method must have no parameter, and return a
Stream.
- * All the test method should be marked with {@link
org.junit.jupiter.api.TestTemplate}, not
- * {@link org.junit.jupiter.api.Test} or {@link
org.junit.jupiter.params.ParameterizedTest}.
+ * providing the arguments if you want to pass parameters to the constructor.
The method must have
+ * no parameter, and return a Stream. For test classes with
no constructor
+ * parameters, omitting the method means running a single invocation. All the
test method should be
+ * marked with {@link org.junit.jupiter.api.TestTemplate}, not {@link
org.junit.jupiter.api.Test} or
+ * {@link org.junit.jupiter.params.ParameterizedTest}.
Review Comment:
Javadoc grammar: "annotation the test class" should be "annotate the test
class" (and similarly "All the test method" -> "All test methods") to keep the
documentation clear and professional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Apache9 commented on PR #8072: URL: https://github.com/apache/hbase/pull/8072#issuecomment-4237383728 Pelase try to fix the javac warnings? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
PDavid commented on PR #8072: URL: https://github.com/apache/hbase/pull/8072#issuecomment-4237170226 Hmm, it seems the java compiler now generates some new warnings. I guess these are not critical. Can you please check them? https://github.com/user-attachments/assets/38a9ab14-68f0-4d18-8880-98541569d723"; /> See: https://github.com/apache/hbase/actions/runs/24324541700?pr=8072 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
liuxiaocs7 commented on code in PR #8072: URL: https://github.com/apache/hbase/pull/8072#discussion_r3072563686 ## hbase-compression/pom.xml: ## @@ -45,6 +45,21 @@ hbase-resource-bundle true + Review Comment: > We do not have junit-vintage dependencies in the past? > > Strange, how could they pass compiling? Yes, previously the `hbase-compression` module did not explicitly reference JUnit-related dependencies... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Apache9 commented on code in PR #8072: URL: https://github.com/apache/hbase/pull/8072#discussion_r3070835381 ## hbase-compression/pom.xml: ## @@ -45,6 +45,21 @@ hbase-resource-bundle true + Review Comment: We do not have junit-vintage dependencies in the past? Strange, how could they pass compiling? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] HBASE-30079 Upgrade hbase-compression to use junit5 [hbase]
Copilot commented on code in PR #8072:
URL: https://github.com/apache/hbase/pull/8072#discussion_r3070712199
##
hbase-server/src/test/java/org/apache/hadoop/hbase/io/compress/HFileTestBase.java:
##
@@ -86,30 +86,30 @@ public void doTest(Configuration conf, Path path,
Compression.Algorithm compress
HFile.Reader reader = HFile.createReader(FS, path, cacheConf, true, conf);
try {
scanner = reader.getScanner(conf, false, false);
- assertTrue("Initial seekTo failed", scanner.seekTo());
+ assertTrue(scanner.seekTo(), "Initial seekTo failed");
Review Comment:
`scanner` is initialized to null and later closed unconditionally in the
`finally` block. If `HFile.createReader(...)` or `reader.getScanner(...)`
throws before `scanner` is assigned, this will throw a `NullPointerException`
and can mask the real failure. Consider using try-with-resources for the
reader/scanner (or at least null-checking `scanner`), and close the scanner
before closing the reader.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/io/compress/HFileTestBase.java:
##
@@ -86,30 +86,30 @@ public void doTest(Configuration conf, Path path,
Compression.Algorithm compress
HFile.Reader reader = HFile.createReader(FS, path, cacheConf, true, conf);
try {
scanner = reader.getScanner(conf, false, false);
- assertTrue("Initial seekTo failed", scanner.seekTo());
+ assertTrue(scanner.seekTo(), "Initial seekTo failed");
do {
ExtendedCell kv = scanner.getCell();
-assertTrue("Read back an unexpected or invalid KV",
- testKvs.contains(KeyValueUtil.ensureKeyValue(kv)));
+assertTrue(testKvs.contains(KeyValueUtil.ensureKeyValue(kv)),
+ "Read back an unexpected or invalid KV");
i++;
} while (scanner.next());
} finally {
reader.close();
scanner.close();
}
-assertEquals("Did not read back as many KVs as written", i,
testKvs.size());
+assertEquals(i, testKvs.size(), "Did not read back as many KVs as
written");
// Test random seeks with pread
Random rand = ThreadLocalRandom.current();
LOG.info("Random seeking with " + fileContext);
reader = HFile.createReader(FS, path, cacheConf, true, conf);
try {
scanner = reader.getScanner(conf, false, true);
- assertTrue("Initial seekTo failed", scanner.seekTo());
+ assertTrue(scanner.seekTo(), "Initial seekTo failed");
for (i = 0; i < 100; i++) {
KeyValue kv = testKvs.get(rand.nextInt(testKvs.size()));
-assertEquals("Unable to find KV as expected: " + kv, 0,
scanner.seekTo(kv));
+assertEquals(0, scanner.seekTo(kv), "Unable to find KV as expected: "
+ kv);
}
Review Comment:
In the second readback section, `scanner` is also closed unconditionally in
`finally` after being assigned inside the `try`. If `reader.getScanner(...)`
throws, `scanner` will still be null and `scanner.close()` will NPE. Use
try-with-resources or guard the close with a null check (and close the scanner
before the reader).
##
hbase-compression/hbase-compression-zstd/src/test/java/org/apache/hadoop/hbase/io/compress/zstd/TestHFileCompressionZstd.java:
##
@@ -86,7 +81,7 @@ public void testReconfLevels() throws Exception {
long len_2 = FS.getFileStatus(path_2).getLen();
LOG.info("Level 1 len {}", len_1);
LOG.info("Level 22 len {}", len_2);
-assertTrue("Reconfiguraton with ZSTD_LEVEL_KEY did not seem to work",
len_1 > len_2);
+assertTrue(len_1 > len_2, "Reconfiguraton with ZSTD_LEVEL_KEY did not seem
to work");
Review Comment:
Typo in assertion message: "Reconfiguraton" should be "Reconfiguration".
```suggestion
assertTrue(len_1 > len_2, "Reconfiguration with ZSTD_LEVEL_KEY did not
seem to work");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
