insightsengineering / rlistings

Value formatting and ASCII rendering infrastructure for tables and listings.
https://insightsengineering.github.io/rlistings/
24 stars 5 forks source link

fix verdepcheck #208

Closed pawelru closed 7 months ago

github-actions[bot] commented 7 months ago

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  ----------------------------------------------------
R/paginate_listing.R        47       1  97.87%   101
R/rlistings_methods.R      101      14  86.14%   39, 54, 58, 140-143, 146, 230-236
R/rlistings.R              166      25  84.94%   158-161, 164-167, 206-210, 377-380, 384-387, 420-423
TOTAL                      314      40  87.26%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 0f782cb408ea0395dd695ddfe53c91ab66d57b4b

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 7 months ago

Unit Tests Summary

 1 files   4 suites   5s :stopwatch: 34 tests 27 :white_check_mark:  7 :zzz: 0 :x: 86 runs  75 :white_check_mark: 11 :zzz: 0 :x:

Results for commit 0f782cb4.

:recycle: This comment has been updated with latest results.

pawelru commented 7 months ago

Currently, both "minimal" and "release" strategies fails the test with the following:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-paginate_listing.R:231'): paginate_to_mpfs works with wrapping on keycols ──
sapply(...) (`actual`) not equal to rep(5, 5) (`expected`).

  `actual`: 4 6 4 6 5
`expected`: 5 5 5 5 5
── Failure ('test-paginate_listing.R:280'): paginate_to_mpfs works with wrapping on keycols when doing horizontal pagination ──
sapply(...) (`actual`) not equal to rep(5, 10) (`expected`).

  `actual`: 4 4 6 6 4 4 6 6 5 5
`expected`: 5 5 5 5 5 5 5 5 5 5

[ FAIL 2 | WARN 0 | SKIP 11 | PASS 73 ]
Error: Test failures

The "maximal" works fine.

For me, this looks very much formatters-related. If the development version is used (as in "maximal" strategy) - everything is fine. I ran the test locally which confirms my thinking. However, I am not fully convinced as I cannot relate this to a recent changes in formatters but I am definitely missing some knowledge here. Calling @insightsengineering/nest-sme for help on this (identical to the similar PR in rtables).

Melkiades commented 7 months ago

Currently, both "minimal" and "release" strategies fails the test with the following:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-paginate_listing.R:231'): paginate_to_mpfs works with wrapping on keycols ──
sapply(...) (`actual`) not equal to rep(5, 5) (`expected`).

  `actual`: 4 6 4 6 5
`expected`: 5 5 5 5 5
── Failure ('test-paginate_listing.R:280'): paginate_to_mpfs works with wrapping on keycols when doing horizontal pagination ──
sapply(...) (`actual`) not equal to rep(5, 10) (`expected`).

  `actual`: 4 4 6 6 4 4 6 6 5 5
`expected`: 5 5 5 5 5 5 5 5 5 5

[ FAIL 2 | WARN 0 | SKIP 11 | PASS 73 ]
Error: Test failures

The "maximal" works fine.

For me, this looks very much formatters-related. If the development version is used (as in "maximal" strategy) - everything is fine. I ran the test locally which confirms my thinking. However, I am not fully convinced as I cannot relate this to a recent changes in formatters but I am definitely missing some knowledge here. Calling @insightsengineering/nest-sme for help on this (identical to the similar PR in rtables).

This is related to the pagination + wrapping fix I did recently. I am happy to see that the tests I added are spotting this instantly. As I mentioned in the other PR in {rtables}, changes in {formatters} are only tested on simplified and limited contexts, while important regression tests are in {rlistings} or {rtables} where issues usually are filed.

I think we need to keep the version updated and, if possible, always have maximal relationship with {formatters}