[SQL] Duplicate test exception in SQLQueryTestSuite due to meta files(.DS_Store) on Mac#17060
[SQL] Duplicate test exception in SQLQueryTestSuite due to meta files(.DS_Store) on Mac#17060dilipbiswal wants to merge 2 commits intoapache:masterfrom
Conversation
|
Test build #73449 has finished for PR 17060 at commit
|
| private val blackList = Set( | ||
| "blacklist.sql" // Do NOT remove this one. It is here to test the blacklist functionality. | ||
| "blacklist.sql", // Do NOT remove this one. It is here to test the blacklist functionality. | ||
| ".ds_store" // A meta-file that may be created on Mac by Finder App. |
There was a problem hiding this comment.
.DS_Store right? I know you compare lower-case later but might as well write it as it appears
There was a problem hiding this comment.
@srowen I did think about it. If we want to keep the mixed case here in this list, then we have to change the code later to lowercase both the sides and then compare ? Would you prefer to change the comparison to the following ?
if (blackList.exists(t => testCase.name.toLowerCase.contains(t.toLowerCase))) {
...
}
There was a problem hiding this comment.
That's right, you'd have to lower-case both. I think that's clearer.
|
|
||
| private def createScalaTestCase(testCase: TestCase): Unit = { | ||
| if (blackList.contains(testCase.name.toLowerCase)) { | ||
| if (blackList.exists(testCase.name.toLowerCase.contains(_))) { |
There was a problem hiding this comment.
Nit, you can remove the (_) here
| val resultFile = file.getAbsolutePath.replace(inputFilePath, goldenFilePath) + ".out" | ||
| TestCase(file.getName, file.getAbsolutePath, resultFile) | ||
| val absPath = file.getAbsolutePath | ||
| TestCase(absPath.stripPrefix(inputFilePath + File.separator), absPath, resultFile) |
There was a problem hiding this comment.
@srowen I am adding a separator to make sure the test case name does not have a leading separator character. Example:
file = /home/spark/sql/core/src/test/resources/sql-tests/inputs/subquery/exists-subquery/exists-basic.sql
inputFilePath = /home/spark/sql/core/src/test/resources/sql-tests/inputs
So i wanted the test case name to be subquery/exists-subquery/exists-basic.sql and not /subquery/exists-subquery/exists-basic.sql. Please let me know what you think.
There was a problem hiding this comment.
Does this get into problems where inputFilePath might already have a trailing separator and then isn't a prefix? maybe it can't happen, but maybe it's easier to strip leading separators from the final result, directly, if that's what you mean to do.
There was a problem hiding this comment.
@srowen Sure. Although i think we are safe, i would go ahead and make that change.
|
Test build #73463 has finished for PR 17060 at commit
|
|
cc @srowen addressed your comments. Could you please check again ? |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
Looks OK. I wonder if we should exclude all hidden files, but, this is more targeted I guess.
|
Thanks! Merging to master. |
|
Thank you @srowen @gatorsmile |
…(.DS_Store) on Mac ## What changes were proposed in this pull request? After adding the tests for subquery, we now have multiple level of directories under "sql-tests/inputs". Some times on Mac while using Finder application it creates the meta data files called ".DS_Store". When these files are present at different levels in directory hierarchy, we get duplicate test exception while running the tests as we just use the file name as the test case name. In this PR, we use the relative file path from the base directory along with the test file as the test name. Also after this change, we can have the same test file name under different directory like exists/basic.sql , in/basic.sql. Here is the truncated output of the test run after the change. ```SQL info] SQLQueryTestSuite: [info] - arithmetic.sql (5 seconds, 235 milliseconds) [info] - array.sql (536 milliseconds) [info] - blacklist.sql !!! IGNORED !!! [info] - cast.sql (550 milliseconds) .... .... .... [info] - union.sql (315 milliseconds) [info] - subquery/.DS_Store !!! IGNORED !!! [info] - subquery/exists-subquery/.DS_Store !!! IGNORED !!! [info] - subquery/exists-subquery/exists-aggregate.sql (2 seconds, 451 milliseconds) .... .... [info] - subquery/in-subquery/in-group-by.sql (12 seconds, 264 milliseconds) .... .... [info] - subquery/scalar-subquery/scalar-subquery-predicate.sql (7 seconds, 769 milliseconds) [info] - subquery/scalar-subquery/scalar-subquery-select.sql (4 seconds, 119 milliseconds) ``` Since this is a simple change, i haven't created a JIRA for it. ## How was this patch tested? Manually verified. This is change to test infrastructure Author: Dilip Biswal <dbiswal@us.ibm.com> Closes apache#17060 from dilipbiswal/sqlquerytestsuite.
4 participants
What changes were proposed in this pull request?
After adding the tests for subquery, we now have multiple level of directories under "sql-tests/inputs". Some times on Mac while using Finder application it creates the meta data files called ".DS_Store". When these files are present at different levels in directory hierarchy, we get duplicate test exception while running the tests as we just use the file name as the test case name. In this PR, we use the relative file path from the base directory along with the test file as the test name. Also after this change, we can have the same test file name under different directory like exists/basic.sql , in/basic.sql. Here is the truncated output of the test run after the change.
Since this is a simple change, i haven't created a JIRA for it.
How was this patch tested?
Manually verified. This is change to test infrastructure