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

Prevent unwanted product test duplicated runs #20321

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 10, 2024

Verify that a product test has reasonably assigned groups. Incorrectly assigned groups may lead to a test executing too many times, or not executing at all.

relates to #15096, #20320

@cla-bot cla-bot bot added the cla-signed label Jan 10, 2024
@findepi findepi marked this pull request as draft January 10, 2024 12:03
@findepi
Copy link
Member Author

findepi commented Jan 10, 2024

cc @wendigo @kokosing @hashhar

@findepi findepi added test maintenance Project maintenance task no-release-notes This pull request does not require release notes entry labels Jan 10, 2024
@findepi findepi force-pushed the findepi/product-test-groups branch from 50ff1c2 to c7b7ed7 Compare January 10, 2024 13:56
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

very very useful

@findepi
Copy link
Member Author

findepi commented Jan 10, 2024

will convert it from a test listener to a classpath scanner, so that it can be run locally quickly

@findepi findepi force-pushed the findepi/product-test-groups branch 3 times, most recently from d73a81e to 6cc327a Compare January 11, 2024 14:02
@findepi findepi requested a review from hashhar January 11, 2024 14:22
@findepi findepi force-pushed the findepi/product-test-groups branch 2 times, most recently from 207bbc0 to c429eda Compare January 11, 2024 16:24
@findepi findepi marked this pull request as ready for review January 11, 2024 16:30
@findepi
Copy link
Member Author

findepi commented Jan 11, 2024

PTAL

@findepi findepi force-pushed the findepi/product-test-groups branch 3 times, most recently from 12f82ce to dc7515b Compare January 11, 2024 19:50
@github-actions github-actions bot added the hive Hive connector label Jan 11, 2024
@findepi findepi force-pushed the findepi/product-test-groups branch from dc7515b to 276fc34 Compare January 12, 2024 08:31
@findepi
Copy link
Member Author

findepi commented Jan 12, 2024

rebased after the release

@findepi findepi changed the title Verify product test groups Prevent unwanted product test duplicated runs Jan 12, 2024

public class HiveProductTest
extends ProductTest
{
static {
TestGroups.FakeUsageForMavenDependencyChecker.fakeUse();
Copy link
Member

Choose a reason for hiding this comment

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

nit: comment why needed

Copy link
Member Author

Choose a reason for hiding this comment

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

class name is "fake usage for maven dependency checker". that's what i would write in a code comment. should i add more context?

@@ -46,7 +46,7 @@ public final class TestGroups
public static final String OAUTH2_REFRESH = "oauth2_refresh";
public static final String MYSQL = "mysql";
public static final String TRINO_JDBC = "trino_jdbc";
public static final String QUERY_ENGINE = "qe";
public static final String QE = "qe"; // query engine
Copy link
Member

Choose a reason for hiding this comment

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

can you rename to QUERY_ENGINE as a followup?

Copy link
Member Author

Choose a reason for hiding this comment

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

i didn't want to do replacements in convention based tests, because it's more erronous (no static checks).

the convention based need some cleanup and/or pruning (eg #20352)
let's leave it for a follow-up


assertThat(name).as("Name of %s", field)
.isUpperCase()
.matches("[A-Z][_A-Z0-9]+")
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you like single char test group names :P

Copy link
Member Author

Choose a reason for hiding this comment

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

i didn't mean to prohibit them, sorry!
it was just the easiest way to have some validation.

Comment on lines 147 to 152
# Run all tests in the functions and create_table groups
testing/bin/ptl test run \
--environment <environment> \
[--config <environment config>] \
-- -g string_functions,create_tables
-- -g functions,create_tables
```
Copy link
Member

Choose a reason for hiding this comment

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

wrong commit

Copy link
Member Author

Choose a reason for hiding this comment

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

leftover -- i was trying to cleanup groups in convention based tests, but decided it's better not to do this in this PR. will revert.

Remove product test groups that are not used for defining test suites.
Verify that a product test has reasonably assigned groups. Incorrectly
assigned groups may lead to a test executing too many times, or not
executing at all.
Remove product test groups that are not used by any tests. These are
meaningless. They do not need to be defined pro actively.
@findepi findepi force-pushed the findepi/product-test-groups branch from 276fc34 to c125d09 Compare January 12, 2024 13:32
@findepi findepi merged commit fdb8101 into master Jan 12, 2024
4 of 12 checks passed
@findepi findepi deleted the findepi/product-test-groups branch January 12, 2024 13:32
@github-actions github-actions bot added this to the 437 milestone Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector maintenance Project maintenance task 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