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

Markdown summary on GHA #1635

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Markdown summary on GHA #1635

wants to merge 11 commits into from

Conversation

gaborcsardi
Copy link
Member

@gaborcsardi gaborcsardi commented Jun 1, 2022

Cf:#1635 (comment)

  • maybe remove of the emoji from the table head?
  • remove (some?) of the emoji from the table body?
  • better printing of timings
  • check if we can link tests to the source code
  • distinguish between headings of multiple archs
  • "context" should be "file"
  • check if we can include the skip reason and the warning text in the table.
  • make "Test Details" the text of the dropdown.
  • consider removing the timings from the details.
  • consider also showing the slowest tests

@gaborcsardi
Copy link
Member Author

Some or all of this could also live in rcmdcheck, It could read the problems.rds file, which is saved by the check reporter. Then rcmdcheck could have a summary that includes check problems and also tests.

OTOH, for test coverage the table is very useful, because otherwise it is really hard to debug test failures there.

@gaborcsardi gaborcsardi requested a review from hadley June 1, 2022 13:25
@hadley
Copy link
Member

hadley commented Jun 1, 2022

Initial comments on summary at https://github.com/r-lib/testthat/actions/runs/2421804786.

This looks great! Lots of ideas/nitpicks below:

  • I think the emoji + text is too much because it's repeated for every job. I'd suggest maybe just text? (and in title case, not upper case)
  • I think you can remove the emoji from the data rows. Alternatively, for the details, you could just use the emoji (since the number isn't that important at the individual test level)
  • Round time to nearest 0.1?
  • How hard would it be to link the test name to the source of the test?
  • For windows latest (https://github.com/r-lib/testthat/actions/runs/2421804786#summary-6689959244), is it possible to adjust the titles to distinguish the test runs?
  • "context" should be "file" I think
  • How hard would it be to include the skip reason and the warning text? Maybe have another column where you can expand <detail> to get a code block containing what you'd see in the test itself?
  • For the test details, I think you could make "test details" the text of the dropdown, and then omit the heading?
  • The timings probably aren't that useful for the test details since you only see problems, not the bulk of the tests. Alternatively, you could maybe also show the slowest x tests? But maybe that works best in a separate table?

@gaborcsardi
Copy link
Member Author

@hadley Thanks for the feedback, I should have said that this is a first draft. :) Yeah, most of these are possible, many of them I was planning on doing. Others are more challenging, but definitely worth taking a better look. Details soon.

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.

2 participants