Skip to content

Data: Switch tests to JUnit5 + AssertJ-style assertions #10657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nastra merged 5 commits into from
Jul 9, 2024
Merged

Data: Switch tests to JUnit5 + AssertJ-style assertions #10657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0d0f46
Data: Switch tests to JUnit5 + AssertJ-style assertions
attilakreiner Jun 30, 2024
dbee20c
Fix review items
attilakreiner Jul 8, 2024
aab0449
Fix review items 2
attilakreiner Jul 9, 2024
ce6ce0f
Fix review items 3
attilakreiner Jul 9, 2024
f6850dd
Fix review items 4
attilakreiner Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions data/src/test/java/org/apache/iceberg/RecordWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@

import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;

import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.StructLikeWrapper;
import org.junit.Assert;
import org.junit.jupiter.api.Test;

public abstract class RecordWrapperTest {
Expand Down Expand Up @@ -103,7 +103,8 @@ public void testNestedSchema() {
}

private void generateAndValidate(Schema schema) {
generateAndValidate(schema, Assert::assertEquals);
generateAndValidate(
schema, (message, expected, actual) -> assertThat(actual).as(message).isEqualTo(expected));
}

public interface AssertMethod {
Expand Down
56 changes: 25 additions & 31 deletions data/src/test/java/org/apache/iceberg/TestMergingMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@

import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.File;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
Expand All @@ -41,14 +44,11 @@
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.NaNUtil;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

@RunWith(Parameterized.class)
import org.junit.jupiter.api.TestTemplate;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;

@ExtendWith(ParameterizedTestExtension.class)
public abstract class TestMergingMetrics<T> {

// all supported fields, except for UUID which is on deprecation path: see
Expand Down Expand Up @@ -110,22 +110,17 @@ public abstract class TestMergingMetrics<T> {
MAP_FIELD_2,
STRUCT_FIELD);

protected final FileFormat fileFormat;

@Parameterized.Parameters(name = "fileFormat = {0}")
public static Object[] parameters() {
return new Object[] {FileFormat.PARQUET, FileFormat.ORC};
}
protected abstract FileAppender<T> writeAndGetAppender(List<Record> records) throws Exception;

public TestMergingMetrics(FileFormat fileFormat) {
this.fileFormat = fileFormat;
@Parameters(name = "fileFormat = {0}")
public static List<Object> parameters() {
return Arrays.asList(FileFormat.PARQUET, FileFormat.ORC);
}

protected abstract FileAppender<T> writeAndGetAppender(List<Record> records) throws Exception;

@Rule public TemporaryFolder temp = new TemporaryFolder();
@Parameter protected FileFormat fileFormat;
@TempDir protected File tempDir;

@Test
@TestTemplate
public void verifySingleRecordMetric() throws Exception {
Record record = GenericRecord.create(SCHEMA);
record.setField(ID_FIELD.name(), 3);
Expand Down Expand Up @@ -166,7 +161,7 @@ public void verifySingleRecordMetric() throws Exception {
assertBoundValueMatch(0D, lowerBounds, MAP_FIELD_2);
}

@Test
@TestTemplate
public void verifyRandomlyGeneratedRecordsMetric() throws Exception {
// too big of the record count will more likely to make all upper/lower bounds +/-infinity,
// which makes the tests easier to pass
Expand All @@ -192,16 +187,16 @@ public void verifyRandomlyGeneratedRecordsMetric() throws Exception {
.map(Types.NestedField::fieldId)
.forEach(
id ->
Assert.assertNull(
"NaN count for field %s should be null", metrics.nanValueCounts().get(id)));
assertThat(metrics.nanValueCounts().get(id))
.as("NaN count for field %s should be null")
.isNull());
}

private void assertNaNCountMatch(
Long expected, Map<Integer, Long> nanValueCount, Types.NestedField field) {
Assert.assertEquals(
String.format("NaN count for field %s does not match expected", field.name()),
expected,
nanValueCount.get(FIELDS_WITH_NAN_COUNT_TO_ID.get(field)));
assertThat(nanValueCount)
.as(String.format("NaN count for field %s does not match expected", field.name()))
.containsEntry(FIELDS_WITH_NAN_COUNT_TO_ID.get(field), expected);
}

private void assertBoundValueMatch(
Expand All @@ -214,10 +209,9 @@ private void assertBoundValueMatch(
int actualFieldId = FIELDS_WITH_NAN_COUNT_TO_ID.get(field);
ByteBuffer byteBuffer = boundMap.get(actualFieldId);
Type type = SCHEMA.findType(actualFieldId);
Assert.assertEquals(
String.format("Bound value for field %s must match", field.name()),
expected,
byteBuffer == null ? null : Conversions.fromByteBuffer(type, byteBuffer));
assertThat(byteBuffer == null ? null : Conversions.<T>fromByteBuffer(type, byteBuffer))
.as(String.format("Bound value for field %s must match", field.name()))
.isEqualTo(expected);
}

private void populateExpectedValues(
Expand Down
57 changes: 24 additions & 33 deletions data/src/test/java/org/apache/iceberg/TestSplitScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
package org.apache.iceberg;

import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import org.apache.hadoop.conf.Configuration;
import org.apache.iceberg.data.GenericAppenderFactory;
Expand All @@ -32,19 +34,15 @@
import org.apache.iceberg.io.FileAppender;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

@RunWith(Parameterized.class)
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.TestTemplate;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;

@ExtendWith(ParameterizedTestExtension.class)
public class TestSplitScan {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);

private static final long SPLIT_SIZE = 16 * 1024 * 1024;

private static final Schema SCHEMA =
Expand All @@ -53,38 +51,31 @@ public class TestSplitScan {

private Table table;
private File tableLocation;

@Rule public TemporaryFolder temp = new TemporaryFolder();
private List<Record> expectedRecords;

@Parameterized.Parameters(name = "format = {0}")
public static Object[] parameters() {
return new Object[] {"parquet", "avro"};
@Parameters(name = "fileFormat = {0}")
public static List<Object> parameters() {
return Arrays.asList(FileFormat.PARQUET, FileFormat.AVRO);
}

private final FileFormat format;
@Parameter private FileFormat format;
@TempDir private File tempDir;

public TestSplitScan(String format) {
this.format = FileFormat.fromString(format);
}

@Before
@BeforeEach
public void before() throws IOException {
tableLocation = new File(temp.newFolder(), "table");
Copy link
Contributor

@nastra nastra Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files.createTempDirectory(temp, "table").toFile()

Copy link
Contributor Author

@attilakreiner attilakreiner Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

tableLocation = java.nio.file.Files.createTempDirectory(tempDir.toPath(), "table").toFile();
setupTable();
}

@Test
@TestTemplate
public void test() {
Assert.assertEquals(
"There should be 4 tasks created since file size is approximately close to 64MB and split size 16MB",
4,
Lists.newArrayList(table.newScan().planTasks()).size());
assertThat(Lists.newArrayList(table.newScan().planTasks()))
.as(
"There should be 4 tasks created since file size is approximately close to 64MB and split size 16MB")
.hasSize(4);

List<Record> records = Lists.newArrayList(IcebergGenerics.read(table).build());
Assert.assertEquals(expectedRecords.size(), records.size());
for (int i = 0; i < expectedRecords.size(); i++) {
Assert.assertEquals(expectedRecords.get(i), records.get(i));
}
assertThat(records).isEqualTo(expectedRecords);
}

private void setupTable() throws IOException {
Expand All @@ -109,8 +100,8 @@ private void setupTable() throws IOException {
}

private File writeToFile(List<Record> records, FileFormat fileFormat) throws IOException {
File file = temp.newFile();
Assert.assertTrue(file.delete());
File file = File.createTempFile("junit", null, tempDir);
assertThat(file.delete()).isTrue();

GenericAppenderFactory factory =
new GenericAppenderFactory(SCHEMA)
Expand Down
13 changes: 6 additions & 7 deletions data/src/test/java/org/apache/iceberg/data/DataTestHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Map;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
import org.junit.Assert;

public class DataTestHelpers {
private DataTestHelpers() {}
Expand All @@ -44,7 +43,7 @@ public static void assertEquals(Types.StructType struct, Record expected, Record
public static void assertEquals(Types.ListType list, List<?> expected, List<?> actual) {
Type elementType = list.elementType();

Assert.assertEquals("List size should match", expected.size(), actual.size());
assertThat(actual).as("List size should match").hasSameSizeAs(expected);

for (int i = 0; i < expected.size(); i += 1) {
Object expectedValue = expected.get(i);
Expand All @@ -57,7 +56,7 @@ public static void assertEquals(Types.ListType list, List<?> expected, List<?> a
public static void assertEquals(Types.MapType map, Map<?, ?> expected, Map<?, ?> actual) {
Type valueType = map.valueType();

Assert.assertEquals("Map size should match", expected.size(), actual.size());
assertThat(actual).as("Map size should match").hasSameSizeAs(expected);

for (Object expectedKey : expected.keySet()) {
Object expectedValue = expected.get(expectedKey);
Expand Down Expand Up @@ -85,14 +84,14 @@ private static void assertEquals(Type type, Object expected, Object actual) {
case UUID:
case BINARY:
case DECIMAL:
Assert.assertEquals(
"Primitive value should be equal to expected for type " + type, expected, actual);
assertThat(actual)
.as("Primitive value should be equal to expected for type " + type)
.isEqualTo(expected);
break;
case FIXED:
assertThat(expected).as("Expected should be a byte[]").isInstanceOf(byte[].class);
assertThat(expected).as("Actual should be a byte[]").isInstanceOf(byte[].class);
Assert.assertArrayEquals(
"Array contents should be equal", (byte[]) expected, (byte[]) actual);
assertThat(actual).as("Array contents should be equal").isEqualTo(expected);
break;
case STRUCT:
assertThat(expected).as("Expected should be a Record").isInstanceOf(Record.class);
Expand Down
10 changes: 4 additions & 6 deletions data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ public class GenericAppenderHelper {

private final Table table;
private final FileFormat fileFormat;
private final TemporaryFolder tmp;
private final Path temp;
private final Configuration conf;

Expand All @@ -54,19 +53,18 @@ public GenericAppenderHelper(
Table table, FileFormat fileFormat, TemporaryFolder tmp, Configuration conf) {
this.table = table;
this.fileFormat = fileFormat;
this.tmp = tmp;
this.temp = null;
this.temp = tmp.getRoot().toPath();
this.conf = conf;
}

public GenericAppenderHelper(Table table, FileFormat fileFormat, Path temp, Configuration conf) {
this.table = table;
this.fileFormat = fileFormat;
this.tmp = null;
this.temp = temp;
this.conf = conf;
}

@Deprecated
public GenericAppenderHelper(Table table, FileFormat fileFormat, TemporaryFolder tmp) {
this(table, fileFormat, tmp, null);
}
Expand Down Expand Up @@ -111,14 +109,14 @@ public void appendToTable(StructLike partition, List<Record> records) throws IOE

public DataFile writeFile(List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
File file = null != tmp ? tmp.newFile() : File.createTempFile("junit", null, temp.toFile());
File file = File.createTempFile("junit", null, temp.toFile());
assertThat(file.delete()).isTrue();
return appendToLocalFile(table, file, fileFormat, null, records, conf);
}

public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
File file = null != tmp ? tmp.newFile() : File.createTempFile("junit", null, temp.toFile());
File file = File.createTempFile("junit", null, temp.toFile());
assertThat(file.delete()).isTrue();
return appendToLocalFile(table, file, fileFormat, partition, records, conf);
}
Expand Down
Loading