-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[MINOR][SQL] Combine the same codes in test cases #23194
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
Conversation
|
Good catch |
|
ok to test |
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.
Is this change related to this PR?
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.
No, I think it can be modified here,so I changed it by the way.
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.
ditto
|
Test build #99551 has finished for PR 23194 at commit
|
eff036c to
126ca0d
Compare
|
Test build #99587 has finished for PR 23194 at commit
|
|
LGTM, cc @HyukjinKwon. |
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.
How about withEmptyDirInTablePath(dirName: String)?
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.
Ok,thank you !
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.
nit: try {
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.
nit: revert this
126ca0d to
867ee5e
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.
private?
|
LGTM except for minor comments |
867ee5e to
877751b
Compare
|
Test build #99644 has finished for PR 23194 at commit
|
|
Test build #99649 has finished for PR 23194 at commit
|
877751b to
ac2b004
Compare
|
Test build #99652 has finished for PR 23194 at commit
|
|
retest this please |
|
Test build #99656 has finished for PR 23194 at commit
|
|
thanks, merging to master! |
## What changes were proposed in this pull request? In the DDLSuit, there are four test cases have the same codes , writing a function can combine the same code. ## How was this patch tested? existing tests. Closes apache#23194 from CarolinePeng/Update_temp. Authored-by: 彭灿00244106 <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>
What changes were proposed in this pull request?
In the DDLSuit, there are four test cases have the same codes , writing a function can combine the same code.
How was this patch tested?
existing tests.