Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build] Set up publication of Test results as comment on pull-requests #3144

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

HannesWell
Copy link
Contributor

As suggested in #3139 (comment).

@cdietrich, @LorenzoBettini unfortunately this is only picked up after it is submitted to the main branch (since I don't have write access to this repo and the new workflow runs when another one is complted).
But as far as I can tell this is the same config as we use in

@HannesWell
Copy link
Contributor Author

@cdietrich I assume the simplest would be to submit this and fix problems if they occur?

runs-on: ubuntu-latest
steps:
- name: Upload
uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to use an exact tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general that's suggested practice to harden supply-chain security.
Otherwise it would be possible to publish a new artifact under a new tag (which is desired if one just uses the major version).
I can change it to just use actions/upload-artifact@v4. Without dependabot being setup it is otherwise tedious to update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw. since you asked in xtext-reference projects, one can configure dependabot to just update workflows:
https://github.com/eclipse-m2e/m2e-core/blob/master/.github/dependabot.yml

And it can also handle these specific commit-ids.

Copy link
Contributor

@LorenzoBettini LorenzoBettini left a comment

Choose a reason for hiding this comment

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

Let's merge it and see how it goes!

Thanks @HannesWell

@LorenzoBettini LorenzoBettini merged commit 3952e5a into eclipse-xtext:main Aug 11, 2024
8 checks passed
@HannesWell HannesWell deleted the publish-test-results branch August 11, 2024 17:09
@cdietrich
Copy link
Contributor

@HannesWell this seems to have broken the build

Run actions/upload-artifact@v4
With the provided path, there will be 978 files uploaded
Artifact name is valid!
Root directory input is valid!
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

any idea

https://github.com/eclipse/xtext/actions/runs/10334822997/job/28608707561?pr=3143

@cdietrich
Copy link
Contributor

this is from your pr
#3143
lets also check mine.

@cdietrich
Copy link
Contributor

is it possible the matrix and the build-maven-artifacts clash?
will try to provide a pr

HannesWell added a commit to HannesWell/eclipse.xtext that referenced this pull request Aug 11, 2024
The 'Publish Unit Test Results' introduced with
eclipse-xtext#3144
will fail if test-failures are found.
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this pull request Aug 11, 2024
The 'Publish Unit Test Results' introduced with
eclipse-xtext#3144
will fail if test-failures are found.
HannesWell added a commit to HannesWell/eclipse.xtext that referenced this pull request Aug 11, 2024
The 'Publish Unit Test Results' introduced with
eclipse-xtext#3144
will fail if test-failures are found.
@HannesWell
Copy link
Contributor Author

This now works much better with

@LorenzoBettini, @cdietrich I'll answer your remarks here to keep the other PRs clean of this discussion.

In response to #3150 (comment)

@HannesWell @LorenzoBettini do you have an idea why Build / Test Results (pull_request) is marked as failed?

Not yet, but I already noticed this too. :/ Unfortunately I could not really find some debug output from that collection job itself.

My first guess is that it might be the errors during the shutdown of the Maven build JVM as one can see as annotations in https://github.com/eclipse/xtext/actions/runs/10347229487.
(Btw. this should be fixed with the next Tycho release: eclipse-tycho/tycho#3489)
But I also see this e.g. in platform and there assume that's not the cause.

In general it has to be something in the logs that the action interprets as an error.
I'll continue to investigate what that is.

In response to #2976 (comment):

@HannesWell two things I don't understand about test results:

* there's no way to see the actual results? I can't seem to be able to click anything on the results

If there are real test-failures there is a button that leads to the failure output. But in this case I think this isn't a false positive failure.
grafik

* if it works only for PR, then it won't work for committers, since we skip the building for our PRs and only build for pushes

I cannot guarantee it, but I would expect that it also works for PRs of branches from the same repository. The jobs still run, just triggered by the push instead of the PR event. The processing is just triggered after all jobs the in Build workflow are completed, so that should be fine. In case the github action, can for some reason not process this setup used by committers, you could still change the logic and always run the PR-event build and just skipp the push-event build if there is also a PR for it and not vice-versa (like it is now), couldn't you?

Of course, for the second item, we could change our design and also build PRs for committers, cc @cdietrich

Well, if you would ask me in a project where I'm in charge I would suggest everyone to use forks, regardless of that person is committer or not. But I'm not in charge here, so my opinion does not count :) I just find it a bit disturbing to have all the current work of committers in my repo. Of course I can just fetch the master and then get rid of that. Maybe it's just a matter of what one is used to.
Btw. as a committer (i.e. if you have write-access to a repo), you can push to the PR's branch even if its in a fork as long as Edits by maintainers is activated by the PR creator (which is on by default) at the upper right site:
grafik

@HannesWell
Copy link
Contributor Author

I have investigated the uploaded artifacts in detail and there indeed test-failures in the runs. One can find them when searching the build-logs for example for <<< FAILURE.
I have no clue at the moment why the details are not shown. I have to investigate that further.

@LorenzoBettini
Copy link
Contributor

IIRC, that's just an output log, but not a real test failure. Tests reports should not show real failures. Alternatively, we enabled rerun for flaky tests, so that might be a failure in one of the first run failures. If the plug-in is not able to report flakes then it might be useless.

@cdietrich
Copy link
Contributor

2024-08-13T08:08:57.8529568Z Tests run: 11, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 4.456 s <<< FAILURE! -- in org.eclipse.xtext.builder.impl.Bug486584Test
2024-08-13T08:08:57.8531781Z org.eclipse.xtext.builder.impl.Bug486584Test.testNoFullBuildIfAttachmentChangeOnly -- Time elapsed: 0.334 s
2024-08-13T08:08:57.8533795Z org.eclipse.xtext.builder.impl.Bug486584Test.testFullBuildWhenClasspathChanged_1 -- Time elapsed: 0.181 s
2024-08-13T08:08:57.8535944Z org.eclipse.xtext.builder.impl.Bug486584Test.testFullBuildWhenClasspathChanged_2 -- Time elapsed: 0.291 s
2024-08-13T08:08:57.8538021Z org.eclipse.xtext.builder.impl.Bug486584Test.testFullBuildWhenClasspathChanged_3 -- Time elapsed: 0.375 s
2024-08-13T08:08:57.8540017Z org.eclipse.xtext.builder.impl.Bug486584Test.testFullBuildWhenClasspathChanged_4 -- Time elapsed: 0.235 s <<< ERROR!
2024-08-13T08:08:57.8542199Z org.eclipse.emf.common.util.WrappedException: org.eclipse.core.internal.resources.ResourceException: Problems encountered while deleting resources.
2024-08-13T08:08:57.8544219Z 	at org.eclipse.xtext.util.Exceptions.throwUncheckedException(Exceptions.java:27)

maybe test does not work on windows

@HannesWell
Copy link
Contributor Author

IIRC, that's just an output log, but not a real test failure. [...] If the plug-in is not able to report flakes then it might be useless.

The test-report.xml shows failure in it's headers:

<testsuite xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd"
version="3.0"
name="org.eclipse.xtend.ide.tests.compiler.QuickDebugSourceInstallingCompilationParticipantTest"
time="1.324" tests="3" errors="2" skipped="0" failures="0">

That the actual builds do not fail is because of #3148, which I suggested based on the assumption that the test-collect job will fail if there are test-failures. And then one can see immediately from the run results if run-failures are test-failures (only the Publish test result job failed) or if there are for example compiler or other errors (the build jobs fail).
But if one cannot access the details of the test-failures easily it has indeed only a limited advantage. But from all of the Eclipse-SDK projects I know that this workflow works in general.

But surprisingly in #3148 (comment) the result is as expected and also the annotations are applied as expected when one clicks on the details.

Maybe it just didn't work before because not everything was available then? 🤔
But with the following two PRs we can now check both scenarios, a PR from a committer and one external

@HannesWell
Copy link
Contributor Author

Looks like haven't refreshed for too long:
Case one actually looks good now: #3153 (comment)

@LorenzoBettini
Copy link
Contributor

I think the build should fail if there are test failures so the Maven option you added is wrong to me. I'd like to see the environment (build) with test failures, not to have a failure only on the test collect build. For example, we know macos is flaky, so its failure could be ignored most of the time. In the current state, it's harder to detect that. At least, that's what I'd find useful. In the current state I don't see benefits from the test report. At least, that's my POV. Unfortunately, I currently don't have time (holiday) to look into that further. Maybe a test report on a per-build matrix basis would be best. If it's too much work, at least a test report for Linux Java 17 and Linux Java 21.

To me, it's crucial now to remove that Maven option. Personally, I'd have a strong opinion on this point.

@HannesWell
Copy link
Contributor Author

In the current state I don't see benefits from the test report. At least, that's my POV. Unfortunately, I currently don't have time (holiday) to look into that further. Maybe a test report on a per-build matrix basis would be best. If it's too much work, at least a test report for Linux Java 17 and Linux Java 21.

Actually I thought this would be a quick-win since it works good in the project's I'm involved and I actually also didn't want to spend much time on this. But I have no clear idea, why it's different here. Multiple builds should not be a problem (Eclipse-platform also has a matrix build). Maybe it's that there are really multiple jobs not one job executed in a matrix?
I don't think that pushing branches to the repo directly is a problem since it works in that case.

I think the build should fail if there are test failures so the Maven option you added is wrong to me. I'd like to see the environment (build) with test failures, not to have a failure only on the test collect build.
[...]

To me, it's crucial now to remove that Maven option. Personally, I'd have a strong opinion on this point.

In general you can see the originating workflow if the failure annotations work. But of course it's one click more and you don't see it immediately.
Created #3155 to revert that.

@LorenzoBettini
Copy link
Contributor

@HannesWell now the test report job always fails when extracting the archives.. I wonder whether our archives are too big..

@HannesWell
Copy link
Contributor Author

The latest executions of the Publish Unit Test Results in https://github.com/eclipse/xtext/actions failed Archive: test-results-Linux-17.zip because test-results-Linux-17.zip seems to be present twice and then the unzip command asks if conflicting content should be overwritten.
I have not yet checked why linux-17 results are present twice in the first place.

@cdietrich
Copy link
Contributor

Cause there is two Linux builds with two j versions

@LorenzoBettini
Copy link
Contributor

But I thought the archives would be different because of the java version used in the name

@HannesWell
Copy link
Contributor Author

Cause there is two Linux builds with two j versions

Yes, but we have three 'Linux' results: one test-results-Linux-21.zip and two test-results-Linux-17.zip.

If you search e.g. https://github.com/eclipse/xtext/actions/runs/10422949927/job/28868593443 for Archive: test-results- you can see this. But it's only the linux-17 zip that's duplicated 🤔

@cdietrich
Copy link
Contributor

IMG_0018

@cdietrich
Copy link
Contributor

Here I see just 1

@cdietrich
Copy link
Contributor

But why does it show twice here

IMG_0019

@HannesWell
Copy link
Contributor Author

But why does it show twice here

I'm wondering as well why this happens?

Here I see just 1

At this job, in the head-line it says 10 artifacts altough in the bottom only two are listed. For example in:
https://github.com/eclipse/xtext/actions/runs/10421428353

What's definitivly wrong that the Upload event file job is executed in
in https://github.com/eclipse/xtext/actions/runs/10421796888

I forgot the condition for this one.
But no clue if this causes problems.

@LorenzoBettini
Copy link
Contributor

The GitHub event must be published for this action to work properly, if I understand its documentation correctly

@HannesWell
Copy link
Contributor Author

The GitHub event must be published for this action to work properly, if I understand its documentation correctly

Yes, but only once per event. If you push a branch directly to the repo it is currently uploaded twice:

@cdietrich
Copy link
Contributor

curl -H "Accept: application/vnd.github.v3+json" \
     -H "Authorization: token ghp_xxxxxxx" \
     https://api.github.com/repos/eclipse/xtext/actions/runs/10421428353/artifacts | jq -r '.artifacts[] | [.name, .archive_download_url] | @tsv'  | while read artifact 
           do
             IFS=$'\t' read name url <<< "$artifact"
             echo "$name"
           done
Event File
test-results-macOS-17
test-results-Linux-17
test-results-Linux-21
test-results-Windows-17
build-maven-artifacts-test-results-Linux
test-results-Linux-17
test-results-Linux-21
test-results-macOS-17
test-results-Windows-17

@cdietrich
Copy link
Contributor

{
      "id": 1820726158,
      "node_id": "MDg6QXJ0aWZhY3QxODIwNzI2MTU4",
      "name": "test-results-macOS-17",
      "size_in_bytes": 19054144,
      "url": "https://api.github.com/repos/eclipse/xtext/actions/artifacts/1820726158",
      "archive_download_url": "https://api.github.com/repos/eclipse/xtext/actions/artifacts/1820726158/zip",
      "expired": false,
      "created_at": "2024-08-16T16:10:00Z",
      "updated_at": "2024-08-16T16:10:00Z",
      "expires_at": "2024-11-14T15:04:00Z",
      "workflow_run": {
        "id": 10421428353,
        "repository_id": 1553758,
        "head_repository_id": 1553758,
        "head_branch": "cd-mwe-2190-m3",
        "head_sha": "655bfa7fdc070f5c5e6618257cde9f9cebdb2d88"
      }
    },
    {
      "id": 1820312683,
      "node_id": "MDg6QXJ0aWZhY3QxODIwMzEyNjgz",
      "name": "test-results-macOS-17",
      "size_in_bytes": 175945,
      "url": "https://api.github.com/repos/eclipse/xtext/actions/artifacts/1820312683",
      "archive_download_url": "https://api.github.com/repos/eclipse/xtext/actions/artifacts/1820312683/zip",
      "expired": false,
      "created_at": "2024-08-16T14:21:16Z",
      "updated_at": "2024-08-16T14:21:16Z",
      "expires_at": "2024-11-14T14:18:29Z",
      "workflow_run": {
        "id": 10421428353,
        "repository_id": 1553758,
        "head_repository_id": 1553758,
        "head_branch": "cd-mwe-2190-m3",
        "head_sha": "655bfa7fdc070f5c5e6618257cde9f9cebdb2d88"
      }
    },
    ```

@cdietrich
Copy link
Contributor

"id": 10421428353, in the same

@HannesWell
Copy link
Contributor Author

Event File
test-results-macOS-17
test-results-Linux-17
test-results-Linux-21
test-results-Windows-17
build-maven-artifacts-test-results-Linux
test-results-Linux-17
test-results-Linux-21
test-results-macOS-17
test-results-Windows-17

That explains the failure.

But I don't understand why this happens?
Do files with the same name have identical content?
If yes we can maybe just override in case of conflicts or ignore.

Can you also check a run with a PR from a fork? Maybe this indeed caused from pushing to the repo directly.

@HannesWell
Copy link
Contributor Author

"id": 10421428353, in the same

Is that from the single event file?

@cdietrich
Copy link
Contributor

cdietrich commented Aug 16, 2024

workflow run is the same. see json above. sizes are drastically different.
maybe cause i did a rerun?

@cdietrich
Copy link
Contributor

next attempt: #3160

@HannesWell
Copy link
Contributor Author

next attempt: #3160

At least the publication of the test-results seems to work now, but the indication is still not working. Either the indicated test-failures are false (the corresponding build jobs should fail then) or posting the failure annotations does not work.
One reason could be this issue:

But I don't understand why this also happens for PRs from forks. AFAICT the setup is almost the same like e.g. in m2e and there it works as expected: https://github.com/eclipse-m2e/m2e-core

I have to admit that I'm slowly losing my patience with this.
Do you think it is worth it to investigate further, i.e. do you really want this, or should this be reverted?

@cdietrich
Copy link
Contributor

from windows

find . -name "*.txt" | xargs grep "Failures: [1-9]"
./org.eclipse.xtend.ide.tests/target/surefire-reports/org.eclipse.xtend.ide.tests.builder.WorkspaceScenariosTest.txt:Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.600 s <<< FAILURE! -- in org.eclipse.xtend.ide.tests.builder.WorkspaceScenariosTest
./org.eclipse.xtend.ide.tests/target/surefire-reports/org.eclipse.xtend.ide.tests.compiler.ResolvingCrossReferenceDuringIndexingTest.txt:Tests run: 9, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 10.97 s <<< FAILURE! -- in org.eclipse.xtend.ide.tests.compiler.ResolvingCrossReferenceDuringIndexingTest

@cdietrich
Copy link
Contributor

cdietrich commented Aug 17, 2024

i see some -Dmaven.test.failure.ignore=true in the repo. e.g. in maven-build.sh and .github/workflows/maven.yml

@LorenzoBettini
Copy link
Contributor

i see some -Dmaven.test.failure.ignore=true in the repo. e.g. in maven-build.sh and .github/workflows/maven.yml

@cdietrich it's only in the shell script that we don't use anymore.

@HannesWell I'd tend to revert this action that doesn't seem to work well, at least in our configuration.. in the future I can try to look for alternative solutions.

@cdietrich
Copy link
Contributor

2024-08-16T20:35:15.1006049Z
2024-08-16T20:35:15.1006206Z Results:
2024-08-16T20:35:15.1006459Z
2024-08-16T20:35:15.1006621Z Flakes:
2024-08-16T20:35:15.1007477Z org.eclipse.xtend.ide.tests.builder.WorkspaceScenariosTest.testJarWithoutJava
2024-08-16T20:35:15.1016298Z Run 1: WorkspaceScenariosTest.testJarWithoutJava:111 mypack.MyData cannot be resolved to a type., MyData cannot be resolved., Couldn't find the mandatory library 'org.eclipse.xtext.xbase.lib' 2.8.0 or higher on the project's classpath. expected:<2> but was:<3>
2024-08-16T20:35:15.1018339Z Run 2: PASS
2024-08-16T20:35:15.1018541Z
2024-08-16T20:35:15.1019105Z org.eclipse.xtend.ide.tests.compiler.ResolvingCrossReferenceDuringIndexingTest.testResolvingXFunctionTypeRef
2024-08-16T20:35:15.1021759Z Run 1: ResolvingCrossReferenceDuringIndexingTest.testResolvingXFunctionTypeRef:261->testResolvingXFunctionTypeRef:447->assertNoErrorsInWorkspace:460->Assert.assertFalse:65->Assert.assertTrue:42->Assert.fail:89 Couldn't find the mandatory library 'org.eclipse.xtext.xbase.lib' 2.8.0 or higher on the project's classpath.
2024-08-16T20:35:15.1023362Z Run 2: PASS
2024-08-16T20:35:15.1023514Z
2024-08-16T20:35:15.1024095Z org.eclipse.xtend.ide.tests.compiler.ResolvingCrossReferenceDuringIndexingTest.testResolvingXFunctionTypeRef_5
2024-08-16T20:35:15.1026444Z Run 1: ResolvingCrossReferenceDuringIndexingTest.testResolvingXFunctionTypeRef_5:307->testResolvingXFunctionTypeRef:447->assertNoErrorsInWorkspace:460->Assert.assertFalse:65->Assert.assertTrue:42->Assert.fail:89 myannotation.MyAnnotation cannot be resolved to a type.
2024-08-16T20:35:15.1027829Z Run 2: PASS
2024-08-16T20:35:15.1027970Z
2024-08-16T20:35:15.1027976Z
2024-08-16T20:35:15.1028217Z Tests run: 3120, Failures: 0, Errors: 0, Skipped: 53, Flakes: 3
2024-08-16T20:35:15.1028722Z
2024-08-16T20:35:16.2797360Z [INFO] All tests passed

maybe there is something wrong with the flaky annotation

cc @szarnekow

@cdietrich
Copy link
Contributor

@LorenzoBettini
Copy link
Contributor

EnricoMi/publish-unit-test-result-action#539

The issue seems to imply that flaky tests are supported when reported by Maven. So everything should be fine when there are flakes.

@cdietrich
Copy link
Contributor

There is still an open pr for that

@HannesWell
Copy link
Contributor Author

EnricoMi/publish-unit-test-result-action#539

The issue seems to imply that flaky tests are supported when reported by Maven. So everything should be fine when there are flakes.

Now I understand what you mean with flaky tests. I have to admit I didn't use surefire.rerunFailingTestsCount yet respectively wasn't aware of it.
Has one of you already verified (e.g. by inspecting the test-result archives) that this is really the cause? From my understand the publish-unit-test-result-action only operates on them, so the build output should be irrelevant. But I don't know on which files that action operates exactly.

@LorenzoBettini
Copy link
Contributor

The log correctly shows flakes reported my surefire: it also tells you which run finally succeeds, or in some cases they finally fail. So I confirm that Maven and Tycho do the right thing. The above issue has been reopened saying that such flakes are currently not supported by the action. It would be enough the PR doesn't report failures in such cases, but I didn't find a way to achieve that.

@cdietrich
Copy link
Contributor

Guess we could try to xslt / sed the xml tags for flaky to dev null

@HannesWell
Copy link
Contributor Author

Thank you @LorenzoBettini, with #3164, at least the checks are now green again if there is no persistent failure.
When EnricoMi/publish-unit-test-result-action#539 is resolved, #3164 can hopefully be reverted and we get the full benefit from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants