Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Sep 21, 2020

What changes were proposed in this pull request?

This PR adds a GitHub Actions job to run WebUI tests with Chrome.
ChromeUISeleniumSuite and ChromeUIHistoryServerSuite are added to test WebUI with better JavaScript support but they are disabled on the CI environment.
Now that we use GitHub Actions, let's add a job to run them.

Why are the changes needed?

To ensure WebUI related changes can work properly and don't make regressions.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I confirmed the new job works on my repository.
I also confirmed we can download unit-tests.log if the WebUI tests fails.
unit-tests-log-webui

@SparkQA
Copy link

SparkQA commented Sep 21, 2020

Test build #128948 has finished for PR 29827 at commit 1c9af36.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Hi, @sarutak . Are you sure you need to reuse java11-maven- prefix?

export MAVEN_CLI_OPTS="--no-transfer-progress"
mkdir -p ~/.m2
./build/mvn -Dspark.test.webdriver.chrome.driver=/usr/bin/chromedriver \
-Dguava.version=25.0-jre -Djava.version=11 -Dtest.default.exclude.tags= -Dtest=none \
Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 22, 2020

Choose a reason for hiding this comment

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

@sarutak . Apache Spark 3.1's official Guava version is still <guava.version>14.0.1</guava.version>, isn't it?
Why do you piggy-back something important thing silently? :)

Copy link
Member Author

@sarutak sarutak Sep 22, 2020

Choose a reason for hiding this comment

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

Yes. I know the official version of Guava is 14.0.1. Actually, I noticed the current ChromeDriver and RemoteWebDriver can't use guava 14.0.1 so I intended to use 25.0 only for ChromeUISeleniumSuite and ChromeUIHistoryServerSuite.
Do you have any better idea rather than use 25.0?

Copy link
Member

Choose a reason for hiding this comment

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

If the test suite is broken with the default Apache Spark distribution, we need to revert the root cause which broke the test suite. IIRC, this test suite is tested with the default configuration initially.

Actually, I noticed the current ChromeDriver and RemoteWebDriver can't use guava 14.0.1

Do we have a JIRA issue for that test suite failure?

Copy link
Member Author

@sarutak sarutak Sep 22, 2020

Choose a reason for hiding this comment

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

The first time those tests are added, they works with guava 14.0.1.
I believe #28585 (SPARK-31765) requires upgraded RemoteWebDriver and it requires 25.0-jre.
That PR is for a small security issue.

Do we have a JIRA issue for that test suite failure?

Yes. But we might need to modify the title.
https://issues.apache.org/jira/browse/SPARK-31996

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the pointer, @sarutak . BTW, there are many request for Guava update. I believe we can handle this in the same group.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, there are many request for Guava update.

Yeah, I know. So, should we revert that PR? It's a security fix but for the test code, or waiting for Guava updated?

Copy link
Member

Choose a reason for hiding this comment

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

What about just not adding this test? :)
How important is it? does this cover new ground?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be one option but I'm wondering if we can make much better coverage for UI tests somehow.
I often see that UI tests are done by "manual" especially when the change is related to JavaScript.
UISeleniumSuite, HistoryServerSuite and other similar suites are enough for the static part of the UI like but not enough for the dynamic part which dynamically changes DOM or CSS by JavaScript.
Those existing suites use HtmlUnitDriver but its JavaScript support is not enough.
So I think it's better if we can run test with real browsers in the CI.
ChromeDriver is similar to HtmlUnitDriver but it uses the real Chrome so we can test JavaScript related stuffs.
ChromeUISeleniumSute and ChromeUIHistoryServerSuite use ChromeDriver rather than HtmlUnitDriver and all tests in the suites are what need JavaScript support so I think they cover new ground.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Please don't add Guava 25.0-jre test silently, @sarutak . That makes the Apache Spark community confused. We can proceed it officially with an independent JIRA id. It's worth.

sudo apt install google-chrome-stable
sudo apt install chromium-chromedriver
- name: Run WebUI tests with Maven
run: |
Copy link
Member

Choose a reason for hiding this comment

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

I think ideally we should just include ChromeUITesttag in core above without changing the guava version ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I tried to do so first but I noticed those tests need the newer Guava so I made a separate job...

Copy link
Member Author

@sarutak sarutak Sep 22, 2020

Choose a reason for hiding this comment

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

@HyukjinKwon I've tried including ChromeUITest tag in core and running GitHub Actions jobs on my repository but as I expected, tests failed due to the Guava version compatibility.

[info] org.apache.spark.deploy.history.ChromeUIHistoryServerSuite *** ABORTED *** (7 milliseconds)
[info]   java.lang.NoSuchMethodError: com.google.common.base.Preconditions.checkState(ZLjava/lang/String;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V
[info]   at org.openqa.selenium.remote.service.DriverService.findExecutable(DriverService.java:134)
[info]   at org.openqa.selenium.chrome.ChromeDriverService.access$000(ChromeDriverService.java:35)
[info]   at org.openqa.selenium.chrome.ChromeDriverService$Builder.findDefaultExecutable(ChromeDriverService.java:159)
[info]   at org.openqa.selenium.remote.service.DriverService$Builder.build(DriverService.java:355)
[info]   at org.openqa.selenium.chrome.ChromeDriverService.createDefaultService(ChromeDriverService.java:94)
[info]   at org.openqa.selenium.chrome.ChromeDriver.<init>(ChromeDriver.java:157)
[info]   at org.apache.spark.deploy.history.ChromeUIHistoryServerSuite.beforeAll(ChromeUIHistoryServerSuite.scala:38)
[info]   at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:212)
[info]   at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
[info]   at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
[info]   at org.apache.spark.SparkFunSuite.run(SparkFunSuite.scala:61)
[info]   at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:318)
[info]   at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:513)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:296)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:286)
[info]   at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info]   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[info]   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[info]   at java.lang.Thread.run(Thread.java:748)
[error] Uncaught exception when running org.apache.spark.deploy.history.ChromeUIHistoryServerSuite: java.lang.NoSuchMethodError: com.google.common.base.Preconditions.checkState(ZLjava/lang/String;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V
sbt.ForkMain$ForkError: java.lang.NoSuchMethodError: com.google.common.base.Preconditions.checkState(ZLjava/lang/String;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V
	at org.openqa.selenium.remote.service.DriverService.findExecutable(DriverService.java:134)

So, we need to specify 25.0-jre only for tests tagged with ChromeUITest. Or, revert SPARK-31765 as I mentioned here.

@SparkQA
Copy link

SparkQA commented Sep 22, 2020

Test build #128959 has finished for PR 29827 at commit cb6aef4.

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

@SparkQA
Copy link

SparkQA commented Sep 22, 2020

Test build #128958 has finished for PR 29827 at commit 5206561.

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

@dongjoon-hyun
Copy link
Member

@sarutak , @HyukjinKwon , @srowen .
If this is only test issue, I guess we can revisit this after we upgrade Apache Spark's Guava version successfully.

@HyukjinKwon
Copy link
Member

+1 for @dongjoon-hyun's...

@sarutak
Copy link
Member Author

sarutak commented Sep 25, 2020

@dongjoon-hyun I agree.

@github-actions
Copy link

github-actions bot commented Jan 4, 2021

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Jan 4, 2021
@github-actions github-actions bot closed this Jan 5, 2021
@dongjoon-hyun dongjoon-hyun reopened this Jan 5, 2021
@SparkQA
Copy link

SparkQA commented Jan 5, 2021

Test build #133637 has finished for PR 29827 at commit cb6aef4.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 5, 2021

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

@SparkQA
Copy link

SparkQA commented Jan 5, 2021

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

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Apr 16, 2021
@github-actions github-actions bot closed this Apr 17, 2021
@sarutak sarutak deleted the githubactions-chrome-webui-test branch June 4, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants