Skip to content

[SPARK-37223][SQL][TESTS] Fix unit test check in JoinHintSuite#34501

Closed
c21 wants to merge 2 commits intoapache:masterfrom
c21:test-fix
Closed

[SPARK-37223][SQL][TESTS] Fix unit test check in JoinHintSuite#34501
c21 wants to merge 2 commits intoapache:masterfrom
c21:test-fix

Conversation

@c21
Copy link
Contributor

@c21 c21 commented Nov 6, 2021

What changes were proposed in this pull request?

This is to fix the unit test where we should assert on the content of log in JoinHintSuite.

Why are the changes needed?

Improve test.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Changed test itself.

@github-actions github-actions bot added the SQL label Nov 6, 2021
@SparkQA
Copy link

SparkQA commented Nov 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49419/

@SparkQA
Copy link

SparkQA commented Nov 6, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49419/

@SparkQA
Copy link

SparkQA commented Nov 6, 2021

Test build #144950 has finished for PR 34501 at commit 284f6f7.

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

assert(logs.size == 2)
logs.forall(_.contains(s"build left for ${joinType.split("_").mkString(" ")} join."))
assert(logs.size == 2 &&
logs.forall(_.contains(s"build left for ${joinType.split("_").mkString(" ")} join.")))
Copy link
Contributor

Choose a reason for hiding this comment

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

how about

assert(logs.size == 2)
assert(logs.forall(_.contains(s"build left for ${joinType.split("_").mkString(" ")} join.")))

It might be easier to distinguish which statement is false if something goes wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Actually maybe even

logs.forall(log => assert(log.contains...))

Also: assert(logs.size === 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen, @advancedxy - updated, thanks.

@SparkQA
Copy link

SparkQA commented Nov 6, 2021

Test build #144958 has finished for PR 34501 at commit e708eae.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Nov 6, 2021

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Nov 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49429/

@SparkQA
Copy link

SparkQA commented Nov 7, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49430/

@SparkQA
Copy link

SparkQA commented Nov 7, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49429/

@SparkQA
Copy link

SparkQA commented Nov 7, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49430/

@SparkQA
Copy link

SparkQA commented Nov 7, 2021

Test build #144959 has finished for PR 34501 at commit e708eae.

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

@srowen srowen closed this in ddf27bd Nov 7, 2021
@srowen
Copy link
Member

srowen commented Nov 7, 2021

Merged to master

@c21
Copy link
Contributor Author

c21 commented Nov 7, 2021

Thank you @srowen and @advancedxy for review!

@c21 c21 deleted the test-fix branch November 15, 2021 10:40
Sign up for free to 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.

5 participants