Skip to content
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

Jump to bottom

Switch to junit5 for mr #9241

Merged
merged 5 commits into from
Dec 11, 2023
Merged

Switch to junit5 for mr #9241

merged 5 commits into from
Dec 11, 2023

Conversation

lschetanrao
Copy link
Contributor

@lschetanrao lschetanrao commented Dec 7, 2023 •
edited

Closes #9083

The goal here is to switch all imports to Junit5 and to use AssertJ-style assertions.

All test cases except the ones using parameterized tests have been migrated. Once #9161 gets merged, I will make the necessary changes to migrate Parameterized test as well.

@nastra nastra self-requested a review December 7, 2023 18:03
nastra
nastra reviewed Dec 8, 2023
View reviewed changes
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated
@@ -54,9 +53,9 @@ public class TestCatalogs {

private Configuration conf;

@Rule public TemporaryFolder temp = new TemporaryFolder();
@TempDir public Path temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@TempDir public Path temp;
@TempDir private Path temp;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated
Assert.assertEquals(PartitionSpecParser.toJson(SPEC), PartitionSpecParser.toJson(table.spec()));
Assertions.assertThat(SchemaParser.toJson(table.schema()))
.isEqualTo(SchemaParser.toJson(SCHEMA));
Assertions.assertThat(PartitionSpecParser.toJson(table.spec()))
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the statically imported assertThat() method here and everywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Fixed this by doing static import in all changed files.

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated
@@ -198,11 +202,11 @@ public void testCreateDropTableToCatalog() throws IOException {
public void testLoadCatalogDefault() {
String catalogName = "barCatalog";
Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, catalogName);
Assert.assertTrue(defaultCatalog.isPresent());
Assertions.assertThat(defaultCatalog.isPresent()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Assertions.assertThat(defaultCatalog.isPresent()).isTrue();
assertThat(defaultCatalog).isPresent();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this in latest commit 10a0099

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated
@@ -212,11 +216,11 @@ public void testLoadCatalogHive() {
InputFormatConfig.catalogPropertyConfigKey(catalogName, CatalogUtil.ICEBERG_CATALOG_TYPE),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, catalogName);
Assert.assertTrue(hiveCatalog.isPresent());
Assertions.assertThat(hiveCatalog.isPresent()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

please also update all the other places that are similar to this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java Outdated
@@ -230,13 +234,13 @@ public void testLoadCatalogHadoop() {
catalogName, CatalogProperties.WAREHOUSE_LOCATION),
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
Assert.assertTrue(hadoopCatalog.isPresent());
Assertions.assertThat(hadoopCatalog.isPresent()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mr/src/test/java/org/apache/iceberg/mr/hive/TestDeserializer.java Outdated
import org.junit.Assume;
import org.junit.Test;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Assumptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

please use AssertJ assumptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. done

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergFilterFactory.java Outdated
assertEquals(actual.child().op(), expected.child().op());
assertEquals(childExpressionActual.ref().name(), childExpressionExpected.ref().name());
assertEquals(childExpressionActual.literal(), childExpressionExpected.literal());
Assertions.assertThat(expected.op()).isEqualTo(actual.op());
Copy link
Contributor

Choose a reason for hiding this comment

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

it's ok to statically import assertThat() here and in the other files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Got it.

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergOutputCommitter.java Outdated
@@ -80,7 +79,7 @@ public class TestHiveIcebergOutputCommitter {
private static final PartitionSpec PARTITIONED_SPEC =
PartitionSpec.builderFor(CUSTOMER_SCHEMA).bucket("customer_id", 3).build();

@Rule public TemporaryFolder temp = new TemporaryFolder();
@TempDir public Path temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@TempDir public Path temp;
@TempDir private Path temp;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergOutputCommitter.java Outdated
JobConf conf = jobConf(table, 1);

Assertions.assertThatThrownBy(
() -> writeRecords(table.name(), 1, 0, true, false, conf, failingCommitter))
.isInstanceOf(RuntimeException.class)
.hasMessage(exceptionMessage);

Assert.assertEquals(1, argumentCaptor.getAllValues().size());
Assertions.assertThat(argumentCaptor.getAllValues().size()).isEqualTo(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as previously mentioned about size assertions: hasSize(xyz) is better here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this in 10a0099

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergSerDe.java Outdated

public class TestHiveIcebergSerDe {

private static final Schema schema =
new Schema(required(1, "string_field", Types.StringType.get()));

@Rule public TemporaryFolder tmp = new TemporaryFolder();
@TempDir public Path tmp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@TempDir public Path tmp;
@TempDir private Path tmp;

Copy link
Contributor

Choose a reason for hiding this comment

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

please also update this in all the other files being touched by this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

nastra
nastra reviewed Dec 9, 2023
View reviewed changes
mr/src/test/java/org/apache/iceberg/mr/hive/TestDeserializer.java Outdated
Assume.assumeFalse(
"No test yet for Hive3 (Date/Timestamp creation)", HiveVersion.min(HiveVersion.HIVE_3));
assumeThat(HiveVersion.min(HiveVersion.HIVE_3))
.as("No test yet for Hive3 (Date/Timestamp creation)");
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs a .isFalse() at the end, otherwise it wouldn't anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I missed that. Thank you for noticing.

@nastra
Copy link
Contributor

nastra commented Dec 9, 2023

@lschetanrao this just needs one minor update around the assumption. Can you also please open an issue to address parameterized tests in this module?

@lschetanrao
Copy link
Contributor Author

@lschetanrao this just needs one minor update around the assumption. Can you also please open an issue to address parameterized tests in this module?

Yes sure.

nastra
nastra approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, the CI failure around the labeler is being fixed separately and is not related to the changes introduced here

@nastra nastra merged commit f21199d into apache:main Dec 11, 2023
10 of 11 checks passed
This was referenced Dec 11, 2023
Closed
Closed
lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@lschetanrao @lisirrx
MR: Migrate tests to JUnit5 (apache#9241)
c394a07
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@lschetanrao @devangjhabakh
MR: Migrate tests to JUnit5 (apache#9241)
4e4d76a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nastra nastra nastra approved these changes

Assignees
No one assigned
Labels
build MR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iceberg-mr: Switch tests to JUnit5 + AssertJ-style assertions
2 participants
@lschetanrao @nastra