Skip to content

Comments

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

Merged
rdblue merged 2 commits intoapache:masterfrom
pvary:splittests
Jan 6, 2021
Merged

Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)#2030
rdblue merged 2 commits intoapache:masterfrom
pvary:splittests

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