-
Notifications
You must be signed in to change notification settings - Fork 5.5k
chore: Cleanup native tests #26154
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
base: master
Are you sure you want to change the base?
chore: Cleanup native tests #26154
Conversation
Reviewer's GuideThis PR streamlines the native test suite by removing the unused implicitCastCharNToVarchar toggle, consolidating query runner creation through a builder pattern, and moving custom-functions helpers into their dedicated test class. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
46e8e92 to
207b037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (this.useExternalWorkerLauncher) { | ||
| externalWorkerLauncher = getExternalWorkerLauncher("hive", serverBinary, cacheMaxSize, remoteFunctionServerUds, | ||
| pluginDirectory, failOnNestedLoopJoin, coordinatorSidecarEnabled, builtInWorkerFunctionsEnabled, enableRuntimeMetricsCollection, enableSsdCache, implicitCastCharNToVarchar); | ||
| pluginDirectory, failOnNestedLoopJoin, coordinatorSidecarEnabled, builtInWorkerFunctionsEnabled, enableRuntimeMetricsCollection, enableSsdCache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets leave the implicitCastCharNToVarchar flag. Since its used in Meta production and likely elsewhere it would be good to try in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please take another look @aditi-pandit? Thanks.
207b037 to
cd3afd2
Compare
cd3afd2 to
c0b0788
Compare
Description
Cleans up tests from
presto-native-tests.Motivation and Context
implicitCastCharNToVarcharconfig is not toggled in any of the native-tests and should not be a parameter in the tests.Helper methods used to test custom functions are only used in
TestCustomFunctionsand should not be a part ofNativeTestUtils.Builder pattern should be used to get the
queryRunnerused in tests and functionNativeTestUtils.createNativeQueryRunnershould not be overloaded with different arguments.Impact
NA