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

Avoid redundantly doubled test setup #19421

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 17, 2023

Disable AbstractTestQueryFramework.ensureTestNamingConvention TestNG test to avoid redundant test setups.

For many test classes, this it the last method that is not converted. For example, running TestInformationSchemaConnector with TestNG will run only this method. Disabling this will make tests like TestInformationSchemaConnector JUnit-only and improve test overall time (fewer createQueryRunner invocations).

The test remains enabled for BaseConnectorTest and BaseConnectorSmokeTest, which already use JUnit and thus are either all-JUnit or do setup twice anyway.

This change also makes it easier to run all-JUnit test classes from an IDE, since now IDE will offer only the "Run with JUnit" option.

Alternative to #19400.

Disable `AbstractTestQueryFramework.ensureTestNamingConvention` TestNG
test to avoid redundant test setups.

For many test classes, this it the last method that is not converted.
For example, running `TestInformationSchemaConnector` with TestNG will
run only this method. Disabling this will make tests like
`TestInformationSchemaConnector` JUnit-only and improve test overall
time (fewer `createQueryRunner` invocations).

The test remains enabled for `BaseConnectorTest` and
`BaseConnectorSmokeTest`, which already use JUnit and thus are either
all-JUnit or do setup twice anyway.

This change also makes it easier to run all-JUnit test classes from an
IDE, since now IDE will offer only the "Run with JUnit" option.
@findepi findepi requested a review from wendigo October 17, 2023 08:57
@cla-bot cla-bot bot added the cla-signed label Oct 17, 2023
@@ -270,7 +270,8 @@ private static String createQueryDebuggingSummary(BasicQueryInfo basicQueryInfo,
}
}

@Test
// TODO @Test - Temporarily disabled to avoid test classes running twice. Re-enable once all tests migrated to JUnit.
Copy link
Member

Choose a reason for hiding this comment

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

issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

we will remember -- when cleaning classpath from testng-related stuff

@findepi
Copy link
Member Author

findepi commented Oct 17, 2023

test (plugin/trino-iceberg, cloud-tests) timed out, but it also times out on master.
eg currently penultimate build https://github.com/trinodb/trino/actions/runs/6537894847/job/17752876156

@findepi findepi merged commit ee11b22 into master Oct 17, 2023
88 of 89 checks passed
@findepi findepi deleted the findepi/avoid-redundantly-doubled-test-setup-0baa45 branch October 17, 2023 11:20
@github-actions github-actions bot added this to the 430 milestone Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants