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

Rename VarcharsTest to match Surefire naming pattern #536

Merged
merged 1 commit into from Apr 2, 2019

Conversation

3 participants
@electrum
Copy link
Member

electrum commented Mar 25, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Mar 25, 2019

@dain
Copy link
Member

dain left a comment

The rename only moved the file and didn't rename the class

@electrum electrum force-pushed the electrum:varcharstest branch from b76cecf to e30b6d6 Mar 25, 2019

@electrum

This comment has been minimized.

Copy link
Member Author

electrum commented Mar 25, 2019

Thanks, fixed

@findepi

This comment has been minimized.

Copy link
Member

findepi commented Mar 25, 2019

@electrum there are quite a few classes named XxxTest (especially in product tests world). You may want to update them too.

@electrum

This comment has been minimized.

Copy link
Member Author

electrum commented Mar 26, 2019

@findepi The only instances I could find were product tests. The root POM changes the Surefire configuration to only check for Test*, but I don't know if that also applies to product tests. If it does, that means those classes aren't being run today (which is the case for VarcharsTest).

There were many other classes that end in Test, such as DataTypeTest, BaseStatsCalculatorTest, AbstractStatisticsBuilderTest, etc., but those aren't test classes. Plus classes like TestThriftIntegrationSmokeTest which also start with it.

@findepi

This comment has been minimized.

Copy link
Member

findepi commented Mar 26, 2019

We could

  • register Test* and *Test in surefire
  • have a check at runtime (should be easy with a testng listener loaded via service loader) that enforces test naming convention

This way, we would close the loop and make it highly unlikely to have tests which are not run.

@dain

dain approved these changes Apr 2, 2019

@electrum electrum merged commit dc11eba into prestosql:master Apr 2, 2019

1 check passed

verification/cla-signed
Details

@electrum electrum deleted the electrum:varcharstest branch Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.