kevinrr888 commented on code in PR #5740:
URL: https://github.com/apache/accumulo/pull/5740#discussion_r2213438077
##########
core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:
##########
@@ -117,6 +142,15 @@ public void testAllColumns() {
DIRECTORY_COLUMN.put(mutation, new Value("t-0001757"));
FLUSH_COLUMN.put(mutation, new Value("6"));
TIME_COLUMN.put(mutation, new Value("M123456789"));
+ OPID_COLUMN.put(mutation,
+ new Value("SPLITTING:FATE:META:12345678-9abc-def1-2345-6789abcdef12"));
+ SELECTED_COLUMN.put(mutation,
+ new Value(new SelectedFiles(Set.of(new ReferencedTabletFile(
+ new
Path("hdfs://nn.somewhere.com:86753/accumulo/tables/42/t-0000/F00001.rf"))
+ .insert()), true, fateId1, SteadyTime.from(100,
TimeUnit.NANOSECONDS))
+ .getMetadataValue()));
+ AVAILABILITY_COLUMN.put(mutation,
TabletAvailabilityUtil.toValue(TabletAvailability.ONDEMAND));
+ MERGEABILITY_COLUMN.put(mutation, new
Value("{\"delay\":1,\"steadyTime\":1,\"never\"=false}"));
Review Comment:
Some of these can be extracted to variables to be used here and in the below
assertions to avoid copy-pastes
##########
core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:
##########
@@ -210,6 +258,11 @@ public void testAllColumns() {
assertEquals(ecMeta.toJson(),
tm.getExternalCompactions().get(ecid).toJson());
assertEquals(10, tm.getFlushNonce().getAsLong());
assertEquals(tsi, tm.getMigration());
+
+ /*
+ * Testing that each
+ */
+ allColumns.forEach(columnType ->
assertTrue(currentTestedColumns.contains(columnType)));
Review Comment:
Would be good to alert the dev in the assertion failure message that the
column does not have testing here, and that they should add testing. Could then
remove the above comment
##########
core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:
##########
@@ -106,6 +126,11 @@ public class TabletMetadataTest {
@Test
public void testAllColumns() {
+ Set<ColumnType> currentTestedColumns = new
HashSet<>(Arrays.asList(LOCATION, PREV_ROW, FILES, LAST, LOADED, SCANS, DIR,
+ TIME, CLONED, FLUSH_ID, FLUSH_NONCE, LOGS, SUSPEND, ECOMP, MERGED,
AVAILABILITY, HOSTING_REQUESTED, OPID,
+ SELECTED, COMPACTED, USER_COMPACTION_REQUESTED, UNSPLITTABLE,
MERGEABILITY, MIGRATION));
+ List<ColumnType> allColumns = List.of(ColumnType.values());
Review Comment:
Apologies, I suggested something like `currentTestedColumns`, but realize it
would be simpler if we just have one collection `allColumns` and just remove
from this as the column is tested. Then at the end we just assert that the
collection is empty. This will:
* Show the dev all of the currently untested columns instead of failing at
the first untested col
* Be a bit more traceable for where the columns are tested
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
