-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-22093][TESTS] Fixes assume in UtilsSuite and HiveDDLSuite
#19332
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
|
@vanzin, Could you take a look when you are available please? |
srowen
left a comment
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.
Yeah I wonder if there are more typos like this. There are a few legitimate assumes about windows
|
Will check other assumes too soon and be back. |
|
I checked other instances of |
|
Test build #82114 has finished for PR 19332 at commit
|
assume in the tests for Utils.resolveURIs and Utils.resolveURIassume in the tests for Utils.resolveURIs, Utils.resolveURI and HiveDDLSuite
assume in the tests for Utils.resolveURIs, Utils.resolveURI and HiveDDLSuiteassume in UtilsSuite and HiveDDLSuite
|
Test build #82117 has finished for PR 19332 at commit
|
dongjoon-hyun
left a comment
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.
+1, LGTM.
|
Merged to master. Thank you @srowen and @dongjoon-hyun. |
What changes were proposed in this pull request?
This PR proposes to remove
assumeinUtils.resolveURIsand replaceassumetoassertinUtils.resolveURIin the test cases inUtilsSuite.It looks
Utils.resolveURIssupports multiple but also single paths as input. So, it looks not meaningful to check if the input has,.For the test for
Utils.resolveURI, I replaced it toassertbecause it looks taking single path and in order to prevent future mistakes when adding more tests here.For
assumeinHiveDDLSuite, it looks it should beassertto test at the lastHow was this patch tested?
Fixed unit tests.