Skip to content

[SPARK-28894][SQL][TESTS] Add a clue to make it easier to debug via Jenkins's test results - #25630

Closed
HyukjinKwon wants to merge 3 commits into
apache:masterfrom
HyukjinKwon:SPARK-28894-1
Closed

[SPARK-28894][SQL][TESTS] Add a clue to make it easier to debug via Jenkins's test results#25630
HyukjinKwon wants to merge 3 commits into
apache:masterfrom
HyukjinKwon:SPARK-28894-1

Conversation

@HyukjinKwon

@HyukjinKwonHyukjinKwon commented Aug 30, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

See https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/109834/testReport/junit/org.apache.spark.sql/SQLQueryTestSuite/

Screen Shot 2019-08-28 at 4 08 58 PM

<?xml version="1.0" encoding="UTF-8"?>
<testsuitehostname="C02Y52ZLJGH5"name="org.apache.spark.sql.SQLQueryTestSuite"tests="3"errors="0"failures="0"skipped="0"time="14.475">
...
<testcaseclassname="org.apache.spark.sql.SQLQueryTestSuite"name="sql - Scala UDF"time="6.703">
</testcase>
<testcaseclassname="org.apache.spark.sql.SQLQueryTestSuite"name="sql - Regular Python UDF"time="4.442">
</testcase>
<testcaseclassname="org.apache.spark.sql.SQLQueryTestSuite"name="sql - Scalar Pandas UDF"time="3.33">
</testcase>
<system-out/>
<system-err/>
</testsuite>

Root cause seems a bug in SBT - it truncates the test name based on the last dot.

sbt/sbt#2949
https://github.com/sbt/sbt/blob/v0.13.18/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala#L71-L79

I tried to find a better way but couldn't find. Therefore, this PR proposes a workaround by appending the test file name into the assert log:

 [info] - inner-join.sql *** FAILED *** (4 seconds, 306 milliseconds)
+ [info] inner-join.sql
[info] Expected "1	a
[info] 1	a
[info] 1	b
[info] 1[]", but got "1	a
[info] 1	a
[info] 1	b
[info] 1[	b]" Result did not match for query #6
[info] SELECT tb.* FROM ta INNER JOIN tb ON ta.a = tb.a AND ta.tag = tb.tag (SQLQueryTestSuite.scala:377)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:528)

It will at least prevent us to search full logs to identify which test file is failed by clicking filed test.

Note that this PR does not fully fix the issue but only fix the logs on its failed tests.

Why are the changes needed?

To debug Jenkins logs easier. Otherwise, we should open full logs and search which test was failed.

Does this PR introduce any user-facing change?

It will print out the file name of failed tests in Jenkins' test reports.

How was this patch tested?

Manually tested but Jenkins tests are required in this PR.

Now it at least shows which file it is:

Screen Shot 2019-08-30 at 10 16 32 PM

@HyukjinKwon

Copy link
Copy Markdown
MemberAuthor

I am going to revert 28e7f2f after we see the test results. It's for testing if the failed tests really have the file name in its assert message.

@HyukjinKwon

Copy link
Copy Markdown
MemberAuthor

// This is a temporary workaround for SPARK-28894. The test names are truncated after
// the last dot due to a bug in SBT. This makes easier to debug via Jenkins test result
// report. See SPARK-28894.
withClue(s"${testCase.name}${System.lineSeparator()}") {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual change is just:

withClue(s"${testCase.name}${System.lineSeparator()}") {
...
}

@SparkQA

Copy link
Copy Markdown

Test build #109942 has finished for PR 25630 at commit 28e7f2f.

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

@HyukjinKwonHyukjinKwon changed the title [WIP][SPARK-28894][SQL][TESTS] Add a clue to make it easier to debug via Jenkins's test results[SPARK-28894][SQL][TESTS] Add a clue to make it easier to debug via Jenkins's test resultsAug 30, 2019
@SparkQA

Copy link
Copy Markdown

Test build #109951 has finished for PR 25630 at commit a3c73c8.

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

@dongjoon-hyun

Copy link
Copy Markdown
Member

@HyukjinKwon

HyukjinKwon commented Aug 30, 2019

Copy link
Copy Markdown
MemberAuthor

Yes but the log on failure test has its file name now -
Screen Shot 2019-08-31 at 7 02 27 AM

I tried to find a better way but couldn't find to fix in Jenkins or SBT. This PR proposes a workaround by appending the test file name into the assert log as I described in PR description.

@dilipbiswal

Copy link
Copy Markdown
Contributor

@HyukjinKwon Its much better than what we had before. thanks !! Looks good to me.

@dongjoon-hyun

Copy link
Copy Markdown
Member

Got it. It's much better! So, in the successful run, there is no information in the last Jenkins. When we have a failure, Error Message will have that information. Did I understand correctly?

@dongjoon-hyun

Copy link
Copy Markdown
Member

I compared both results. +1, LGTM.

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @HyukjinKwon and @dilipbiswal .
Merged to master!

@HyukjinKwon

Copy link
Copy Markdown
MemberAuthor

Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@HyukjinKwon@SparkQA@dongjoon-hyun@dilipbiswal