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.

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

Migrate selected Iceberg tests #19505

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Migrate selected Iceberg tests #19505

merged 3 commits into from
Oct 24, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 24, 2023

No description provided.

it's no longer about only metadata files.
@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Oct 24, 2023
@findepi findepi requested review from wendigo and ebyhr October 24, 2023 07:01
@cla-bot cla-bot bot added the cla-signed label Oct 24, 2023
@github-actions github-actions bot added the iceberg Iceberg connector label Oct 24, 2023
public static Object[][] fileFormats()
{
return Stream.of(IcebergFileFormat.values())
.map(icebergFileFormat -> new Object[] {icebergFileFormat})
.toArray(Object[][]::new);
}

@Test(dataProvider = "fileFormats")
@ParameterizedTest
@MethodSource("fileFormats")
Copy link
Member

Choose a reason for hiding this comment

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

We could use @EnumSource(IcebergFileFormat.class) instead in this class & TestIcebergMigrateProcedure.

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

We don't need separate commits for each test migration. You can have a single commit "Migrate Iceberg tests" and it will still be relatively small and easy to review.

Previously the check allowed e.g. `org.junit.jupiter.api.Test`, but not
e.g. `org.junit.jupiter.params.ParameterizedTest` (different package).
@findepi
Copy link
Member Author

findepi commented Oct 24, 2023

Thank you @ebyhr @electrum for your review .
updated!

@findepi findepi merged commit db9f1b9 into trinodb:master Oct 24, 2023
90 checks passed
@findepi findepi deleted the findepi/ifo branch October 24, 2023 14:11
@github-actions github-actions bot added this to the 431 milestone Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed iceberg Iceberg connector no-release-notes This pull request does not require release notes entry test
Development

Successfully merging this pull request may close these issues.

3 participants