Skip to content

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 gaborcsardi force-pushed the feature/gha-summary branch from 901d2d2 to 574e249 Compare June 1, 2022 10:12
@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.

@hadley
Copy link
Member

hadley commented Aug 1, 2025

Fixes #1621

@hadley
Copy link
Member

hadley commented Aug 15, 2025

I'm not sure how much further to push this — summaries are considerable less useful that I had expected since (as far as I can tell) you need two clicks to get to them from this page and there's no easy way to know that they exist. I think if we were to do more, it would make sense to make this part of the CheckReporter and write hand-in with the console outputs in #2183.

@gaborcsardi
Copy link
Member Author

Yeah, I forgot if it is was possible to have a single summary for all runs, probably not? If not, then I don't really want this, it is too much to have a separate summary for each job, even if we improve the output.

Maybe we could have a job that summarizes the tests of all jobs? But then this does not really belong in testthat.

@hadley
Copy link
Member

hadley commented Aug 28, 2025

Yeah, my sense is that we've explored this idea and it turns out to not be that useful, and there's no point in putting more time into it.

@hadley hadley closed this Aug 28, 2025
@hadley hadley mentioned this pull request Aug 28, 2025
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