Anirban166 / Autocomment-atime-results

GitHub Action that automatically comments a plot and other atime-based results on PRs
https://github.com/marketplace/actions/autocomment-atime-results
0 stars 1 forks source link

show tests_preview_facet.png in PR comment, link to tests_all_facet.png #37

Closed tdhock closed 1 month ago

tdhock commented 1 month ago

Since https://github.com/tdhock/atime/commit/ea4103f797aac2dbcae5c6858f9cc29b386d87a6 there is an output of a new image file, tests_preview_facet.png which only shows the 4 tests with the most significant differences between HEAD and min. this is available using current atime on CRAN. @Anirban166 can you please modify the PR comment so that tests_preview_facet.png is shown, but then when you click on it, you can see tests_all_facet.png ?

Anirban166 commented 1 month ago

@Anirban166 can you please modify the PR comment so that tests_preview_facet.png is shown, but then when you click on it, you can see tests_all_facet.png ?

Done! (a while ago) You can see it working here, but why is tests_preview_facet.png horizontally stretched? (you can download the zip containing the results and check the same file to verify it's an issue with the image itself)

It looks a bit weird, but if you're fine with it then I can send a PR to data.table today to update my action with this feature and #36 combined. Otherwise I'll send it after you've fixed that, just let me know!

which only shows the 4 tests with the most significant differences between HEAD and min.

Just my take, but I think 3 tests might be better

tdhock commented 1 month ago

thanks! the "horizontally stretched" issue was because the width was based on the total number of test cases, not the number of tests shown in the preview PNG. I won't publish another atime to CRAN for a while, but please do submit a data.table PR with your new changes. 3 tests instead of 4? that can be configured by defining N.tests.preview in atime/tests.R https://github.com/tdhock/binsegRcpp/commit/f70739aa3995b7f7553ccf4160afa370d5d5f9d7