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: Migrate subclasses of FlinkCatalogTestBase to JUnit5 #9381

Merged
merged 7 commits into from Jan 13, 2024
Merged

Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 #9381

merged 7 commits into from Jan 13, 2024

Conversation

vinitpatni
Copy link
Contributor

  • Added junit 5 conversion and AssertJ style for TestFlinkCatalogTable and TestFlinkMetadataTable

@github-actions github-actions bot added the flink label Dec 26, 2023
@vinitpatni vinitpatni mentioned this pull request Dec 26, 2023
Merged
nastra
nastra reviewed Dec 29, 2023
View reviewed changes
flink/v1.18/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java Outdated
"Should have the expected columns",
ImmutableSet.of("data", "id"),
ImmutableSet.copyOf(uniqueConstraintOptional.get().getColumns()));
assertThat(uniqueConstraintOptional.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.

ack

@vinitpatni
Copy link
Contributor Author

  • Adding Junit 5 conversion and AssertJ style for TestFlinkUpsert and TestRewriteDataFilesAction

@vinitpatni
Copy link
Contributor Author

@nastra TestStreamScanSql is the only subclass remaining for conversion to Junit 5 but it has dependency on GenericAppenderHelper class which is part of iceberg-data module. Let me know how to tackle that ?

@vinitpatni vinitpatni mentioned this pull request Jan 2, 2024
Closed
@nastra
Copy link
Contributor

nastra commented Jan 5, 2024

@vinitpatni the issue with GenericAppenderHelper has been addressed in #9185, so this class should now be usable with JUnit4 + JUni5

@vinitpatni
Copy link
Contributor Author

I have done changes in TestStreamScanSql locally but running testcases on my local machine is taking lot of time. It is running indefinitely for this class. @nastra Do you have any idea ?

@nastra
Copy link
Contributor

nastra commented Jan 9, 2024

I have done changes in TestStreamScanSql locally but running testcases on my local machine is taking lot of time. It is running indefinitely for this class. @nastra Do you have any idea ?

@vinitpatni without seeing the changes for TestStreamScanSql it's difficult to say what's wrong there. Can you please push your changes for this test? The changes in the other files LGTM

@vinitpatni
Copy link
Contributor Author

  • Adding Junit 5 conversion and AssertJ style for TestStreamScanSql.

@nastra I tried to execute locally both junit 4 version (main branch version) as well as this version(junit5 one). These tests are running indefinitely for both of these versions

@vinitpatni vinitpatni marked this pull request as ready for review January 9, 2024 09:25
nastra
nastra reviewed Jan 10, 2024
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.

almost ready to go, just 2 minor things to address

@vinitpatni
Copy link
Contributor Author

  • Addressing Review Comments

nastra
nastra approved these changes Jan 10, 2024
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, thanks @vinitpatni. Could you also please remove FlinkCatalogTestBase as part of this PR as I don't think it's used anymore, since everything was converted.

@nastra nastra changed the title Adding Junit 5 conversion and AssertJ style for TestFlinkCatalogTable… Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 Jan 10, 2024
@vinitpatni
Copy link
Contributor Author

LGTM, thanks @vinitpatni. Could you also please remove FlinkCatalogTestBase as part of this PR as I don't think it's used anymore, since everything was converted.

Ack. Removed FlinkCatalogTestBase and its references

nastra
nastra approved these changes Jan 13, 2024
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.

thanks @vinitpatni for getting this done

@nastra nastra merged commit 850cd5c into apache:main Jan 13, 2024
13 checks passed
@vinitpatni vinitpatni deleted the junit5-flinkcatalogtestbase branch January 14, 2024 16:11
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
@vinitpatni @geruh
Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 (apache#9381
1cac336 
)
adnanhemani pushed a commit to adnanhemani/iceberg that referenced this pull request Jan 30, 2024
@vinitpatni @adnanhemani
Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 (apache#9381
546396a 
)
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Jan 31, 2024
@rodmeneses
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses o…
ad7cca9 
…f FlinkCatalogTestBase to JUnit5
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Jan 31, 2024
@rodmeneses
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses o…
4c893d6 
…f FlinkCatalogTestBase to JUnit5
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Jan 31, 2024
@rodmeneses
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses o…
47b29c8 
…f FlinkCatalogTestBase to JUnit5
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Feb 1, 2024
@rodmeneses
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses o…
ca78564 
…f FlinkCatalogTestBase to JUnit5
pvary pushed a commit that referenced this pull request Feb 2, 2024
@rodmeneses
Flink: backport #9381 to 1.17 and 1.16 for Migrate subclasses of Fli…
2c24750 
…nkCatalogTestBase to JUnit5 (#9598)
rodmeneses added a commit to rodmeneses/iceberg that referenced this pull request Feb 2, 2024
@rodmeneses
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses …
d1144bc 
…of FlinkCatalogTestBase to JUnit5 (apache#9598)
rodmeneses pushed a commit to rodmeneses/iceberg that referenced this pull request Feb 19, 2024
@vinitpatni @rodmeneses
Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 (apache#9381
b5835f6 
)
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@vinitpatni @devangjhabakh
Flink: Migrate subclasses of FlinkCatalogTestBase to JUnit5 (apache#9381
f606ace 
)
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@rodmeneses @devangjhabakh
Flink: backport apache#9381 to 1.17 and 1.16 for Migrate subclasses …
84059e7 
…of FlinkCatalogTestBase to JUnit5 (apache#9598)
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