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

Flink: Create CatalogTestBase for migration to JUnit5 #9364

Merged
merged 6 commits into from
Dec 26, 2023
Merged

Flink: Create CatalogTestBase for migration to JUnit5 #9364

merged 6 commits into from
Dec 26, 2023

Conversation

vinitpatni
Copy link
Contributor

Base class creation of FlinkCatalogTestBase for the migration to JUnit5 in regards to #9079

@github-actions github-actions bot added the flink label Dec 22, 2023
nastra
nastra reviewed Dec 22, 2023
View reviewed changes
flink/v1.18/flink/src/test/java/org/apache/iceberg/flink/FlinkCatalogTestBaseJU5.java Outdated
import org.junit.jupiter.api.io.TempDir;

@ExtendWith(ParameterizedTestExtension.class)
public abstract class FlinkCatalogTestBaseJU5 extends TestBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just call this CatalogTestBase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@nastra
Copy link
Contributor

nastra commented Dec 22, 2023

can you also please convert TestMetadataTableReadableMetrics to JUnit5 and use CatalogTestBase as its new base class?

@vinitpatni
Copy link
Contributor Author

I tested this implementation with couple of subclasses and it seems to be working fine. The difference that I observed here as compared to junit4 in terms of test execution. First test is triggered with all the supplied parameters in serial order then only it proceeds to next test in the sequence.

@vinitpatni
Copy link
Contributor Author

can you also please convert TestMetadataTableReadableMetrics to JUnit5 and use CatalogTestBase as its new base class?

ack

@nastra
Copy link
Contributor

nastra commented Dec 22, 2023

I tested this implementation with couple of subclasses and it seems to be working fine. The difference that I observed here as compared to junit4 in terms of test execution. First test is triggered with all the supplied parameters in serial order then only it proceeds to next test in the sequence.

Can you include the subclasses you tested in this PR and convert them to JUnit5 in a separate commit?

@vinitpatni
Copy link
Contributor Author

Thanks for the review, @nastra I have added following changes

  • Rename FlinkCatalogTestBaseJU5 to CatalogTestBase
  • Added Junit5 testcase and AssertJ style for TestFlinkCatalogDatabase

@vinitpatni
Copy link
Contributor Author

Addressed Following review comments

  • Using static imports
  • Removing redundant newline
  • Using Assumptions from Assertj library

nastra
nastra reviewed Dec 23, 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.

I've left a few minor comments, can you also please convert TestMetadataTableReadableMetrics and include it in this PR? In a follow-up PR we should then include all the other tests that currently extent FlinkCatalogTestBase and convert them to use CatalogTestBase.

nastra
nastra reviewed Dec 23, 2023
View reviewed changes
flink/v1.18/flink/src/test/java/org/apache/iceberg/flink/CatalogTestBase.java Outdated
protected boolean isHadoopCatalog;

@Parameters(name = "catalogName={0} baseNamespace={1}")
static List<Object[]> parametrs() {
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
static List<Object[]> parametrs() {
protected static List<Object[]> parameters() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@nastra nastra changed the title Create FlinkCatalogTestBaseJU5 for migration to JUnit5 Flink: Create CatalogTestBase for migration to JUnit5 Dec 23, 2023
Addressing Review Comments and Adding Junit 5 conversion for TestMeta…
eb53f8b 
…dataTableReadableMetrics and TestFlinkCatalogTablePartitions
@vinitpatni
Copy link
Contributor Author

  • Addressed Review Comments for CatalogTestBase and TestFlinkCatalogDatabase
  • Moved parameters method to subclass of CatalogTestBase as there can be only single parameterProviders which will be defined by subclass extending CatalogTestBase
  • Adding Junit 5 conversion and AssertJ style for TestMetadataTableReadableMetrics and TestFlinkCatalogTablePartitions

@vinitpatni
Copy link
Contributor Author

  • Revert as per original behaviour and moved parameters to CatalogTestBase and overrided the parameters in subclass as per need

nastra
nastra reviewed Dec 24, 2023
View reviewed changes
flink/v1.18/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogDatabase.java Outdated
assertThat(validationNamespaceCatalog.namespaceExists(icebergNamespace))
.as("Namespace should exist")
.isTrue();
assertThat(sql("SHOW TABLES").size()).as("Should not list any tables").isEqualTo(0);
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
assertThat(sql("SHOW TABLES").size()).as("Should not list any tables").isEqualTo(0);
assertThat(sql("SHOW TABLES")).isEmpty();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

nastra
nastra reviewed Dec 24, 2023
View reviewed changes
...18/flink/src/test/java/org/apache/iceberg/flink/source/TestMetadataTableReadableMetrics.java Outdated
@@ -76,7 +72,7 @@ protected TableEnvironment getTableEnv() {
return super.getTableEnv();
}

@Rule public TemporaryFolder temp = new TemporaryFolder();
@TempDir 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.

this can be private (unless there's a subclass that might use this)

nastra
nastra reviewed Dec 24, 2023
View reviewed changes
...18/flink/src/test/java/org/apache/iceberg/flink/source/TestMetadataTableReadableMetrics.java Outdated
@@ -134,8 +130,7 @@ private Table createPrimitiveTable() throws IOException {
createPrimitiveRecord(
false, 2, 2L, Float.NaN, 2.0D, new BigDecimal("2.00"), "2", null, null));

DataFile dataFile =
FileHelpers.writeDataFile(table, Files.localOutput(temp.newFile()), records);
DataFile dataFile = FileHelpers.writeDataFile(table, Files.localOutput(temp.toFile()), records);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be doing File.createTempFile("junit", null, temp.toFile()) instead of temp.toFile(), similar to how it was done in #9341. The same needs to be done in L151

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@nastra
Copy link
Contributor

nastra commented Dec 24, 2023

@vinitpatni just a few minor things, but should be ready to go once those are addressed

@nastra nastra mentioned this pull request Dec 24, 2023
Merged
@vinitpatni
Copy link
Contributor Author

  • Addressed Review comments

@vinitpatni vinitpatni marked this pull request as ready for review December 24, 2023 14:33
@nastra nastra merged commit 6c344db into apache:main Dec 26, 2023
13 checks passed
@vinitpatni
Copy link
Contributor Author

@nastra Thanks for merging my changes. I have also completed Junit 5 conversion and AssertJ style for TestFlinkCatalogTable and TestFlinkMetadataTable. Should I created Separate PR or follow up on this one ?

@nastra
Copy link
Contributor

nastra commented Dec 26, 2023

Just create a new PR

@vinitpatni
Copy link
Contributor Author

@nastra Please find this new PR. #9381

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@vinitpatni @lisirrx
Flink: Create CatalogTestBase for migration to JUnit5 (apache#9364)
0e00732
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
@vinitpatni @geruh
Flink: Create CatalogTestBase for migration to JUnit5 (apache#9364)
b11a591
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Jan 31, 2024
@rodmeneses
Flink: Backport apache#9364 to 1.16 and 1.17 for Create CatalogTestBa…
74ccc53 
…se for migration to JUnit5
pvary pushed a commit that referenced this pull request Feb 1, 2024
@rodmeneses
Flink: Backport #9364 to 1.16 and 1.17 for Create CatalogTestBase for…
61532a0 
… migration to JUnit5 (#9601)
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Feb 1, 2024
@rodmeneses
Flink: Backport apache#9364 to 1.16 and 1.17 for Create CatalogTestBa…
7a0147a 
…se for migration to JUnit5
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Feb 2, 2024
@rodmeneses
Flink: Backport apache#9364 to 1.16 and 1.17 for Create CatalogTestBa…
ce9b792 
…se for migration to JUnit5
rodmeneses pushed a commit to rodmeneses/iceberg that referenced this pull request Feb 19, 2024
@vinitpatni @rodmeneses
Flink: Create CatalogTestBase for migration to JUnit5 (apache#9364)
d7b5f5a
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Feb 19, 2024
@rodmeneses
Flink: Backport apache#9364 to 1.16 and 1.17 for Create CatalogTestBa…
0de75f3 
…se for migration to JUnit5 (apache#9601)
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@vinitpatni @devangjhabakh
Flink: Create CatalogTestBase for migration to JUnit5 (apache#9364)
a55d681
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@rodmeneses @devangjhabakh
Flink: Backport apache#9364 to 1.16 and 1.17 for Create CatalogTestBa…
5644887 
…se for migration to JUnit5 (apache#9601)
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
flink
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vinitpatni @nastra