Skip to content

New IT Framework - InputSource and InputFormat Tests#13597

Merged
cryptoe merged 6 commits intoapache:masterfrom
abhagraw:InputSourceAndFormat
Jan 4, 2023
Merged

New IT Framework - InputSource and InputFormat Tests#13597
cryptoe merged 6 commits intoapache:masterfrom
abhagraw:InputSourceAndFormat

Conversation

@abhagraw
Copy link
Contributor

@abhagraw abhagraw commented Dec 19, 2022

Migrating InputSource and InputFormat Tests to the new IT framework.

The following tests are being migrated to the new framework -

ITHttpInputSourceTest.java
ITLocalInputSourceAllInputFormatTest.java
ITSqlInputSourceTest.java

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Minor comments. WIll merge after CICD

* under the License.
*/

package org.apache.druid.testsEx.indexer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this indexer-specific test? Should this package be something else like inputsource

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an existing package. I believe "indexer" is the name of the service that does ingestion, not just the name of a process that manages tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was following the existing structure. We can create a new package if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh maybe we can change it in another PR .

@cryptoe
Copy link
Contributor

cryptoe commented Dec 19, 2022

@abhagraw Also please mention the exact tests which are being migrated as part of the PR description for the trail.

Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for doing the conversion. For this one, it appears we were able to reuse the code pretty much as-is. That is a good outcome: it allows tests to be converted rapidly.

I wonder, as we convert, should we remove the old tests? Or, at least leave a comment indicating that they have been converted? Doing so will make it easier to track the conversions as we move beyond having just a handful.

Other than the formatting issue pointed out below, LGTM.

* under the License.
*/

package org.apache.druid.testsEx.indexer;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an existing package. I believe "indexer" is the name of the service that does ingestion, not just the name of a process that manages tasks.

@abhagraw
Copy link
Contributor Author

Looks good. Thanks for doing the conversion. For this one, it appears we were able to reuse the code pretty much as-is. That is a good outcome: it allows tests to be converted rapidly.

I wonder, as we convert, should we remove the old tests? Or, at least leave a comment indicating that they have been converted? Doing so will make it easier to track the conversions as we move beyond having just a handful.

Other than the formatting issue pointed out below, LGTM.

@paul-rogers Currently we only run tests on jvm 8 using the new framework. We need to add support for running on jvm 11 to remove the old framework tests completely.

@cryptoe cryptoe merged commit 365474f into apache:master Jan 4, 2023
@cryptoe
Copy link
Contributor

cryptoe commented Jan 4, 2023

Thanks, @abhagraw for migrating these tests!!

@abhagraw abhagraw deleted the InputSourceAndFormat branch January 4, 2023 05:16
@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants