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

Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924) #2030

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

pvary
Copy link
Contributor

@pvary pvary commented Jan 5, 2021

As mentioned in #1924 this PR separates out the test cases needing a different combination of parameters

@github-actions github-actions bot added the MR label Jan 5, 2021
@rdblue
Copy link
Contributor

rdblue commented Jan 5, 2021

Looks like this is really effective at reducing build time! I'd still like to eliminate a lot of the tests that aren't adding much value. We should have tests for Hive reading formats and tests for Hive working with tables, but we don't need all the format tests run on all the tables, or all the table tests run across all formats and engines.

@rdblue
Copy link
Contributor

rdblue commented Jan 6, 2021

JDK 8 tests run in 36 minutes! Great work, @pvary!

@rdblue rdblue merged commit 9c5948f into apache:master Jan 6, 2021
@pvary pvary deleted the splittests branch January 7, 2021 08:13
@pvary
Copy link
Contributor Author

pvary commented Jan 7, 2021

Thanks for the review and the merge @rdblue!

With this runtimes do you think we still need to separate out the integration tests?
We might just keep everything as it is for now for simplicity shake, and if we see higher runtimes later then we can revisit the integration test idea?

@rdblue
Copy link
Contributor

rdblue commented Jan 7, 2021

With this runtimes do you think we still need to separate out the integration tests?

No, I think this is okay now. If we wanted to run all combinations, I'd say that is good for integration tests. But as long as we're careful about what we're testing and reducing the combinations if they don't provide much value we are good.

XuQianJin-Stars pushed a commit to XuQianJin-Stars/iceberg that referenced this pull request Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants