[DO-NOT-MERGE][WIP][MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins #25923

HeartSaVioR · 2019-09-25T03:07:13Z

NOTE: I'll change the title and fill out content if it works as expected

What changes were proposed in this pull request?

...TBD...

Why are the changes needed?

...TBD...

Does this PR introduce any user-facing change?

No

How was this patch tested?

Amplab Jenkins build will test the patch.

HeartSaVioR · 2019-09-25T03:13:25Z

I'm not sure we should force test names to not contain . (maybe also < and > as well) only for sbt (& Jenkins build), but as it would definitely help on tracking individual test in SQLQuerySuite & ThriftServerQueryTestSuite, I guess it worths to do this.

HyukjinKwon · 2019-09-25T03:17:06Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

      val testCaseName = absPath.stripPrefix(inputFilePath).stripPrefix(File.separator)
+        .replace('.', '_')


@HeartSaVioR, this way affects individual file testing way. This is a bug in SBT and it affects all tests (see #25630)

We should rather find a way to override SBT's builtin XML reporter but looks not easy.

Or we should find a way to use Scalatest's XML reporter but that seems not working with Java tests given my rough test.

@HyukjinKwon
Ah OK. Thanks for pointing out. Looks like the code relies on testCase.name in some places.

Would it work if we replace the '.' character with '_' just when we pass the name to test method? I meant here:

// Create a test case to run this case. test(testCase.name) { runTest(testCase) }

Ah I just saw your comments after commenting. Yeah it's ideal if sbt fixes the issue but looks like they don't mind.

IMHO we should apply the workaround for now if we create dynamic tests which end up with same name like here. We may not want to address all the occurrence (like single test having '.') as it's still trackable but this case they're all marked as sql and not trackable in Jenkins.

It affects the test way of selecting individual SQL test files because --z option is based upon test name. I mean this:

spark/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

Line 52 in febac9f

* build/sbt "~sql/test-only *SQLQueryTestSuite -- -z inline-table.sql"

Yes, we might also need to guide it as well. Assuming we don't have another . in test file name, can we just remove postfix of .sql and update javadoc? I hope the change is acceptable, given the change is deterministic and intuitive.

For adding context, this is to help tracking the issue related to #25913. subquery/scalar-subquery/scalar-subquery-select.sql is failing, but as we know it's just represented as sql and bunch of tests are having same name which bothers with weird UI impact in Jenkins page (clicking sql in Jenkins page is indeterministic now), as well as hard to track among with multiple builds.

HeartSaVioR · 2019-09-25T04:19:04Z

FYI, we know about sbt/sbt#2949 which failed to get attention, but new issue sbt/sbt#5114 is filed again which somehow seemed to get attention from repository owner, but owner asked for contribution instead of fixing it by their hands and reporter denied - so kind of stuck. If someone is interested and ambitious about this issue, please go get it.

Some funny thing is, JUnitXmlReportPlugin is described as "experimental" but they enable it by default. We still can't disable this plugin given #25923 (comment)

https://github.com/sbt/sbt/blob/0e69402660685dcd4b12293cce365772b837a0f3/main/src/main/scala/sbt/plugins/JUnitXmlReportPlugin.scala#L14-L24

/**
 * An experimental plugin that adds the ability for junit-xml to be generated.
 *
 *  To disable this plugin, you need to add:
 *  {{{
 *     val myProject = project in file(".") disablePlugins (plugins.JunitXmlReportPlugin)
 *  }}}
 *
 *  Note:  Using AutoPlugins to enable/disable build features is experimental in sbt 0.13.5.
 */
object JUnitXmlReportPlugin extends AutoPlugin {

SparkQA · 2019-09-25T06:07:15Z

Test build #111321 has finished for PR 25923 at commit febac9f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2019-09-25T06:27:24Z

Regarding build failure for febac9f :

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111321/testReport

- org.apache.spark.sql.SQLQueryTestSuite.blacklist_sql

 Error Details
java.io.FileNotFoundException: /home/jenkins/workspace/SparkPullRequestBuilder@3/sql/core/src/test/resources/sql-tests/results/blacklist.sql.out (No such file or directory)

I guess that's fixed in f714453 but let me confirm with the build result.

SparkQA · 2019-09-25T07:05:02Z

Test build #111327 has finished for PR 25923 at commit f714453.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-09-25T09:21:27Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

@@ -49,7 +50,7 @@ import org.apache.spark.tags.ExtendedSQLTest
 *
 * To run a single test file upon change:
 * {{{
- *   build/sbt "~sql/test-only *SQLQueryTestSuite -- -z inline-table.sql"


Not sure about this because practically it was good since file names can be matched. Now it cannot be for cosmetic reasons.

I added a clue in the PR I pointed out in order to make debugging easier with the Jenkins output in Github. So currently people at least can identify which file was failed.

Huge win of this workaround is availability of looking into history of individual test. It would provide context which cannot be retrieve from test log, which is really helpful to track down test flakiness.
e.g. https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111332/testReport/org.apache.spark.sql/SQLQueryTestSuite/typeCoercion_native_mapconcat/history/

Full tests in SQLQueryTestSuite:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111332/testReport/org.apache.spark.sql/SQLQueryTestSuite/

Full tests in ThriftServerQueryTestSuite (same test can be matched with SQLQueryTestSuite):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111332/testReport/org.apache.spark.sql.hive.thriftserver/ThriftServerQueryTestSuite/

So I'm afraid I'm not sure it's just only cosmetic issue. This workaround brings actual benefits, though it has pros and cons, unfortunately. I guess someone would be familiar with file path instead of file path excluding ".sql", so please treat this as a proposal and weigh on current state vs "workaround applied" state.

But this affects existing test way. Can't we find a way to override SBT's XML reporter or to use ScalaTest's XML reporter?

Fixing this in SBT, and upgrading the version should be the standard approach.

I totally agree that's the way to go, but if no one would like to go forward, that's just an ideal approach. (And that doesn't seem easy to fix the sbt issue with ensuring it doesn't break anything.) sbt/sbt#2949 is filed at Feb. 2017, which is over 2 years ago.

Yes. For the current approach in this PR, I am still not sure due to the downside of changing the existing test way.

I don't think many people actually know build/sbt "~sql/test-only *SQLQueryTestSuite -- -z xxx.sql" is based upon its test name and matching it to the exact file name is pretty good one.

I don't think many people actually know build/sbt "~sql/test-only *SQLQueryTestSuite -- -z xxx.sql" is based upon its test name

I'm not sure I could agree with this, as that means this feature is blindly used at all which it shouldn't (as it only applies to *SQLQueryTestSuite). This is even documented in Spark website. Please refer "Testing with SBT" in http://spark.apache.org/developer-tools.html

matching it to the exact file name is pretty good one.

Yes I totally agree it is more intuitive and better. So that's "trade-off". Test history of *SQLQueryTestSuite in Jenkins is just unusable as of now, so personally I think it's worth to weigh on both. Not a strong opinion as I commented before - it's just a proposal.

SparkQA · 2019-09-25T11:16:36Z

Test build #111332 has finished for PR 25923 at commit b3414a2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2019-09-25T12:57:21Z

I've reached desired result, but it's just a proposal so I'll defer updating title and content of PR until someone supports this idea and would like to merge it.

HeartSaVioR · 2019-09-29T23:14:47Z

Hopefully there's PR sbt/sbt#5139 submitted to address root issue. I'll follow up that PR instead.

margussipria · 2019-09-30T08:02:52Z

I wasn't going to fix that root issue (I just disabled that plugin and used Scalatest provided writer) , but then @HyukjinKwon linked that I wasn't only one with this issue, so I started to make tests to fix this issue (and not make things worst for other test suites)

HeartSaVioR added 2 commits September 25, 2019 11:17

[MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins

e672c67

Add explanation

febac9f

HyukjinKwon reviewed Sep 25, 2019

View reviewed changes

Just applying another approach

f714453

Expand to all cases in SQLQueryTestSuite & ThriftServerQueryTestSuite

b3414a2

HyukjinKwon reviewed Sep 25, 2019

View reviewed changes

dongjoon-hyun added the SQL label Sep 25, 2019

HeartSaVioR closed this Sep 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO-NOT-MERGE][WIP][MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins #25923

[DO-NOT-MERGE][WIP][MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins #25923

HeartSaVioR commented Sep 25, 2019

HeartSaVioR commented Sep 25, 2019 •

edited

Loading

HyukjinKwon Sep 25, 2019

HyukjinKwon Sep 25, 2019

HyukjinKwon Sep 25, 2019

HeartSaVioR Sep 25, 2019

HeartSaVioR Sep 25, 2019

HyukjinKwon Sep 25, 2019 •

edited

Loading

HeartSaVioR Sep 25, 2019 •

edited

Loading

HeartSaVioR Sep 25, 2019 •

edited

Loading

HeartSaVioR commented Sep 25, 2019 •

edited

Loading

SparkQA commented Sep 25, 2019

HeartSaVioR commented Sep 25, 2019

SparkQA commented Sep 25, 2019

HyukjinKwon Sep 25, 2019

HeartSaVioR Sep 25, 2019

HyukjinKwon Sep 26, 2019

HeartSaVioR Sep 26, 2019

HyukjinKwon Sep 26, 2019

HeartSaVioR Sep 26, 2019

SparkQA commented Sep 25, 2019

HeartSaVioR commented Sep 25, 2019

HeartSaVioR commented Sep 29, 2019

margussipria commented Sep 30, 2019

		val testCaseName = absPath.stripPrefix(inputFilePath).stripPrefix(File.separator)
		.replace('.', '_')

[DO-NOT-MERGE][WIP][MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins #25923

[DO-NOT-MERGE][WIP][MINOR][SQL][TESTS] show tests in SQLQuerySuite correctly in Jenkins #25923

Conversation

HeartSaVioR commented Sep 25, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

HeartSaVioR commented Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

HeartSaVioR Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

HeartSaVioR Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

HeartSaVioR commented Sep 25, 2019 • edited Loading

SparkQA commented Sep 25, 2019

HeartSaVioR commented Sep 25, 2019

SparkQA commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 25, 2019

HeartSaVioR commented Sep 25, 2019

HeartSaVioR commented Sep 29, 2019

margussipria commented Sep 30, 2019

HeartSaVioR commented Sep 25, 2019 •

edited

Loading

HyukjinKwon Sep 25, 2019 •

edited

Loading

HeartSaVioR Sep 25, 2019 •

edited

Loading

HeartSaVioR Sep 25, 2019 •

edited

Loading

HeartSaVioR commented Sep 25, 2019 •

edited

Loading