[SPARK-32357][INFRA] Publish failed and succeeded test reports in GitHub Actions

[HyukjinKwon]

What changes were proposed in this pull request?

This PR proposes to report the failed and succeeded tests in GitHub Actions in order to improve the development velocity by leveraging ScaCap/action-surefire-report. See the example below:

Note that we cannot just use ScaCap/action-surefire-report in Apache Spark because PRs are from the forked repository, and GitHub secrets are unavailable for the security reason. This plugin and all similar plugins require to have the GitHub token that has the write access in order to post test results but it is unavailable in PRs.

To work around this limitation, I took this approach:

1. In workflow A, run the tests and upload the JUnit XML test results. GitHub provides to upload and download some files.
2. GitHub introduced new event type workflow_run 10 days ago. By leveraging this, it triggers another workflow B.
3. Workflow B is in the main repo instead of fork repo, and has the write access the plugin needs. In workflow B, it downloads the artifact uploaded from workflow A (from the forked repository).
4. Workflow B generates the test reports to port from JUnit xml files.
5. Workflow B looks up the PR and posts the test reports.

The workflow_run event is very new feature, and looks not so many GitHub Actions plugins support. In order to make this working with ScaCap/action-surefire-report, I had to fork two GitHub Actions plugins to use:

• ScaCap/action-surefire-report to have this custom fix: HyukjinKwon/action-surefire-report@c96094c
  It added commit argument to specify the commit to post the test reports. With workflow_run, it can access, in workflow B, to the commit from workflow A.

• dawidd6/action-download-artifact to have this custom fix: HyukjinKwon/action-download-artifact@750b71a
  It added the support of downloading all artifacts from workflow A, in workflow B. By default, it only supports to specify the name of artifact.

  Note that I was not able to use the official actions/download-artifact because:

  • It does not support to download artifacts between different workflows, see also Download from a different workflow actions/download-artifact#3. Once this issue is resolved, we can switch it back to actions/download-artifact.

I plan to make a pull request for both repositories so we don't have to rely on forks.

Why are the changes needed?

Currently, it's difficult to check the failed tests. You should scroll down long logs from GitHub Actions logs.

Does this PR introduce any user-facing change?

No, dev-only.

How was this patch tested?

Manually tested at: HyukjinKwon#17, HyukjinKwon#18, HyukjinKwon#19, HyukjinKwon#20, and master branch of my forked repository.

[HyukjinKwon]

I am running other tests to make sure it includes PySpark test reports as well in this PR. I will add a comment here once all testing is finished.

Currently, I only did a basic test as described in the PR description. I verified that it includes both JUnit tests and Scala tests.

cc @viirya, @dongjoon-hyun, @gengliangwang, @dbtsai FYI

[HyukjinKwon]

This path is an exact copy from Jenkins configuration.

[gengliangwang]

@HyukjinKwon the report looks great. Thanks for the work!

[gengliangwang]

I saw your comment in #29169 (comment)
Quick question here: Is there another working plugin?

[HyukjinKwon]

Nope.. this one was the only one given my few tries. I'm trying to figure out the github token issue. I'll bring up more info later. I opened some tickets and am discussing it with ASF infra team and GitHub team now.

[viirya]

The report looks nice.

[dongjoon-hyun]

It's great, @HyukjinKwon ! :)

[dbtsai]

Thanks @HyukjinKwon This will speed up our development velocity dramatically.

[HyukjinKwon]

Just to share the current status,

In ScaCap/action-surefire-report plugin (and all other similar plugins), it leverages GITHUB_TOKEN that is set by default in GitHub Actions. It uses GitHub API to create status checks via here - it requires write permission to the repo. However, the permissions of GITHUB_TOKEN does not cover the case when a PR was raised based on a fork.

There are many similar issues and questions, for example, in codecov or GitHub community. In case of Codecov, they managed to remove the requirement of GITHUB_TOKEN at here. Basically they used existing GitHub Actions environment variables to verify in their service. This is not feasible in our case because the plugin is dependent of GitHub API to create the status checks directly.

I investigated this issue yesterday and concluded there's no clean workaround to make this working out of the box.
I am currently investigating the feasibility of potential alternatives. I am not yet sure if all of them work or not:

• Use one environment variable, for example, TEST_REPORT_GITHUB_TOKEN as a GitHub secret. And then, guide committers to set TEST_REPORT_GITHUB_TOKEN as a GitHub secret in their forks so that the PRs report test results. Note that the contributors would not be able to report the test results as their tokens don't have the write access to the repo.

• Just run the test reports only in the commits of the repo and don't run them in PRs until GitHub provides an alternative to work around this. There look many requests such as this.

• Just generate a token that only has the permission to change the status checks, and hardcode it in the repo. At the worst case people abuse this token, the status checks of PRs or commits can be changed. This does not affect the codes and Jenkins runs as a safeguard so it might be fine. I wonder what people can get by abusing this status checks.

I opened an INFRA ticket in ASF and a Github Actions ticket in GitHub, and am discussing the options. Once I verify the feasible options, we will be able to discuss further which one to pick (or just drop the PR at the worst case).

[HyukjinKwon]

BTW, let me know if you guys have a preference in one of the options. I can investigate fewer options in this case.
Adding @srowen, @gatorsmile, @cloud-fan as well to collect more feedback.

[srowen]

If there's not a clean way to do it, and this isn't essential but nice-to-have, then option 1 seems the most viable. It would be opt-in and only for people who want to take the time to set this up, so not sure it would cover much. But probably more useful than doing it only after the fact, or exposing a token.

[HyukjinKwon]

Sure, let me take a closer look for that approach. In worst case, we might have to drop this and go back to the original @viirya's approach at #29169.

[HyukjinKwon]

I am making some changes to demonstrate the problem in terms of the fork and PRs. Please ignore the changes made from now on. I will switch back from the draft later when this is ready for a review.

[HyukjinKwon]

The latest commit above (2688f21) contains the problem in terms of forked repos and PRs at this workload.
The current changes in this PR are exactly same as HyukjinKwon#16 at my own fork and PR in terms of how the ScaCap/action-surefire-report works.

[HyukjinKwon]

Okay, GitHub team suggested a scenario that it could work out of the box. It's a bit complicated then I thought but I will try. Yes, it is a nice-to-have but I think it is very worthwhile as it saves time of all Spark developers in their daily development and review at Apache Spark.

[HyukjinKwon]

I opened a PR to verify if this works well in the main commit (5debde9) and the PR (#29429).

[viirya]

If previous Run tests is passed without failure, do we still need run this? I remember Github Actions has some conditions other than always() can be used?

[HyukjinKwon]

Yeah, if the tests don't fail, it should upload JUnit XML files and then report the successful test cases. e.g.) 1000 tests passed 6 skipped 0 failures.

GitHub Actions has things like failure() but I think we should run this always (to report successful cases and also failed cases).

[HyukjinKwon]

Also, I would like to thank @cpintado from GitHub. He virtually guided me here a lot on this.

[gengliangwang]

A late LGTM. Thanks for the great work!

[maropu]

Thanks, @HyukjinKwon @cpintado ! Great work and the results became very easy to see!!!

[HyukjinKwon]

Okay .. I checked that it works in both PR and commit. There look a bit of delay roughly 2 ~ 5 minutes to have the test report after the main tests are finished but I think this is good enough.