[SPARK-39074][CI] Fail on upload, not download of missing test files

[EnricoMi]

What changes were proposed in this pull request?

The CI should not fail when there are no test result files to be downloaded.

Why are the changes needed?

The CI workflow "Report test results" fails when there are no artifacts to be downloaded from the triggering workflow. In some situations, the triggering workflow is not skipped, but all test jobs are skipped in case no code changes are detected.

Examples:

• https://github.com/G-Research/spark/runs/6208072225?check_suite_focus=true#step:4:14
• https://github.com/G-Research/spark/runs/6094719451?check_suite_focus=true#step:4:14

In that situation, no test files are uploaded, which makes the triggered workflow fail.

Downloading no test files can have two reasons:

1. The code has been build but no tests have been executed, or tests have been executed but no test files have been generated.
2. No code has been built and tested deliberately.

You want to be notified in the first situation to fix the CI. Therefore, CI should fail when code is built and tests are run but no test result files are been found.

The dawidd6/action-download-artifact action cannot be configured to not fail when there are no files. But Github Actions can be configured (continue-on-error) to ignore that and still have the job succeed. However, if that action fails for a different reason, this will not be raised as an error any more.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Not tested

[EnricoMi]

@HyukjinKwon @Yikun there are still errors in forks when syncing main branch.

[AmplabJenkins]

Can one of the admins verify this patch?

[HyukjinKwon]

Hm, there's a case when *.xml are not generated time to time although when the tests passed. My only concern is that to make that case also failed after this change.

[EnricoMi]

You refer to case 1. in the description above?

[HyukjinKwon]

Oh no. I meant that there's a case when the tests actually run but XML files are not generated for some reasons I don't know IIRC.

[EnricoMi]

I think that is what I meant with 1.. I have reworded that to be more clear:

1. The code has been build […] tests have been executed but no test files have been generated.

This is covered by if-no-files-found: error for actions/upload-artifact. The upload action is triggered when tests are run, so it expects files to upload.

[srowen]

I think this is OK? @HyukjinKwon

[HyukjinKwon]

We can try and go ahead, I'm not against this. My only concern is that we'll now consider the build as failed in the case when the build is successful but it failed to generate XML test reports for an unknown reason - this happens often as far as I remember. Though, I agree that this PR can make such unknown cases onto the surface.

[HyukjinKwon]

This change might be fine.

[srowen]

Merged to master

[HyukjinKwon]

Seems like it fails in some cases such as 3b709eb

The problem seems that https://github.com/apache/spark/blob/master/.github/workflows/build_and_test.yml#L134-L142 and actually testing skip logic are not matched.

For example, here https://github.com/apache/spark/runs/6911661666?check_suite_focus=true#step:4:14 it says to build both PySpark and other builds too but later other builds were skipped at https://github.com/apache/spark/runs/6911690622?check_suite_focus=true#step:9:21

We should probably remove pyspark-, sparkr, etc at https://github.com/apache/spark/blob/master/.github/workflows/build_and_test.yml#L135 but I think it needs a closer look.

Reverting this for now to avoid making other PRs failed.

cc @EnricoMi mind taking a look please?

cc @dongjoon-hyun too FYI

[srowen]

Ah shoot OK thank you. You were right

[HyukjinKwon]

that's fine. i think it's good that we know there's a problem there 👍

[dongjoon-hyun]

Thank you for the investigation, @HyukjinKwon .

[EnricoMi]

Yes, it looks like the scope of the if condition of the build jobs is broader than the logic in dev/run-tests.py. The individual build jobs test individual sets of modules (e.g. module sql). They should be skipped if none of these modules have changed.

The Check changes job creates the build output, which covers modules of all build jobs and is used to skip all build jobs. The individual build jobs test far smaller sets of modules, so there may always be modules skipped by dev/run-tests.py though build is true.

This looks like a bigger change.