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

[improve][CI] Replace test reporting with less verbose solution #17319

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Aug 28, 2022

Fixes #16736

Motivation

See #16736

This PR reduces noise in test reporting and removes the misleading check runs from PR checks.

Modifications

@lhotari
Copy link
Member Author

lhotari commented Aug 29, 2022

@tisonkun @maxsxu please join the review too

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Generally looks good. Thanks!

Two comments:

  1. ./.github/actions/merge-test-reports seems no more references, we may remove it.
  2. I don't find a report on the new progress in the demo [improve][CI] Replace test reporting with less verbose solution lhotari/pulsar#78, can you show me a direct link for the test report page? (IIUC it should be something like this one: https://github.com/apache/pulsar/pull/17062/checks?check_run_id=8027361638)

@lhotari
Copy link
Member Author

lhotari commented Aug 29, 2022

Generally looks good. Thanks!

Two comments:

  1. ./.github/actions/merge-test-reports seems no more references, we may remove it.

Yes makes sense.

  1. I don't find a report on the new progress in the demo [improve][CI] Replace test reporting with less verbose solution lhotari/pulsar#78, can you show me a direct link for the test report page? (IIUC it should be something like this one: https://github.com/apache/pulsar/pull/17062/checks?check_run_id=8027361638)

@tisonkun
The goal of the PR is to not add the report as a check run. The report is displayed on the workflow run summary page: https://github.com/lhotari/pulsar/actions/runs/2943996610
Besides the markdown report, annotations are added with the relevant code location and stack trace.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your explanation! I get the point. +1 to merge,

@tisonkun
Copy link
Member

tisonkun commented Aug 29, 2022

One thing to remind here is that we may want to cherry-pick this change to previous release branch. I suppose we can possibly remove the test-reporter action from pulsar-test-infra.

Or we can tag pulsar-test-infra and use a stable version :)

@lhotari
Copy link
Member Author

lhotari commented Aug 29, 2022

One thing to remind here is that we may want to cherry-pick this change to previous release branch. I suppose we can possibly remove the test-reporter action from pulsar-test-infra.

Or we can tag pulsar-test-infra and use a stable version :)

The forks of apache/pulsar still depend on test-reporter action and it might be a good idea to keep it for some time, also after our maintenance branches no more use it.

btw. There would be a way in GitHub Actions to make maintenance branches use workflows defined in the master branch. This is possible with the new reuseable workflows, https://github.blog/changelog/2022-08-22-github-actions-improvements-to-reusable-workflows-2/ . There's a tradeoff in doing this, but that could make things easier if we have a single pipeline to maintain.

Copy link
Contributor

@maxsxu maxsxu left a comment

Choose a reason for hiding this comment

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

Thanks for this clean PR!

Just wondering how we added the action-junit-report against the pulsar-test-infra repo?

@tisonkun
Copy link
Member

@maxsxu I think @lhotari pushed a commit apache/pulsar-test-infra@eeb9107 directly.

@lhotari
Copy link
Member Author

lhotari commented Aug 29, 2022

@maxsxu I think @lhotari pushed a commit apache/pulsar-test-infra@eeb9107 directly.

yes

Copy link
Contributor

@maxsxu maxsxu left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit cb881d4 into apache:master Aug 30, 2022
nicoloboschi pushed a commit that referenced this pull request Sep 9, 2022
Fixes #16736

* [improve][CI] Replace test reporting with less verbose solution

- uses https://github.com/mikepenz/action-junit-report which has been
  copied to https://github.com/apache/pulsar-test-infra/tree/master/action-junit-report

* Address review comment: remove merge-test-reports action

(cherry picked from commit cb881d4)
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.

CI: Replace current GitHub Actions test reporting solution with less verbose solution
5 participants