Skip to content

Comments

Changes from external PR for truetype font#875

Merged
Melkiades merged 12 commits intomainfrom
261_truetype_pag
Jun 4, 2024
Merged

Changes from external PR for truetype font#875
Melkiades merged 12 commits intomainfrom
261_truetype_pag

Conversation

@gmbecker
Copy link
Collaborator

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented May 29, 2024

Unit Tests Summary

    1 files     25 suites   1m 35s ⏱️
  205 tests   205 ✅ 0 💤 0 ❌
1 532 runs  1 532 ✅ 0 💤 0 ❌

Results for commit 733b723.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 29, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
Exporters 💔 $16.90$ $+3.61$ $+1$ $0$ $0$ $0$
Pagination 💔 $14.06$ $+3.40$ $+3$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
Exporters 💔 $2.42$ $+1.24$ export_as_pdf_works
Exporters 💔 $1.20$ $+1.54$ exporting_pdf_does_the_inset
Pagination 👶 $+3.04$ setting_colgap_during_pagination_works

Results for commit ff05373

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 29, 2024

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               751      62  91.74%   20, 94, 97, 412, 499-500, 503, 655, 755, 847-848, 949, 951-952, 970-973, 993, 1102-1105, 1199-1204, 1351, 1451-1454, 1520-1523, 1559-1562, 1568-1573, 1624, 1631, 1725, 1833, 1846, 1849-1852, 1855-1858, 1885, 1916-1917
R/as_html.R                    161      25  84.47%   5-10, 74, 131-136, 141-146, 161-165, 252
R/colby_constructors.R         567      20  96.47%   81, 134, 197-200, 267-270, 404, 420, 1152, 1240, 1401, 1440, 1462, 1486, 1507, 1653
R/compare_rtables.R             83      17  79.52%   93-96, 99-102, 115-118, 137, 156-157, 188, 193
R/format_rcell.R                12       0  100.00%
R/indent.R                      13       2  84.62%   40-41
R/index_footnotes.R             66       0  100.00%
R/make_split_fun.R             138      31  77.54%   22-26, 36-39, 52-55, 58-61, 122, 126, 274, 277-280, 285-288, 302, 372, 381, 383, 385, 436
R/make_subset_expr.R           143      16  88.81%   35, 47-61, 135-142, 156, 179, 259, 275, 284
R/simple_analysis.R              5       1  80.00%   56
R/split_funs.R                 510      66  87.06%   127, 132, 138-143, 156, 173-177, 353-358, 375-380, 456, 502, 518-521, 537, 599, 609-610, 612, 624, 668, 693, 868, 875, 899-902, 913-914, 916, 918, 1090-1092, 1106-1110, 1174-1177, 1240-1243
R/summary.R                    144      38  73.61%   35, 80, 178-220, 269, 315-331, 366, 397
R/tree_accessors.R             948     103  89.14%   113, 267, 287, 313, 354, 372, 390, 503, 530-531, 817-823, 967, 986, 1012, 1064, 1121-1122, 1159, 1194, 1232-1237, 1296, 1370-1374, 1392-1401, 1479, 1594-1597, 1622, 1644-1645, 1655, 1706, 1727-1732, 1753-1758, 1769, 1844, 1885, 1981, 2082, 2095, 2109, 2125, 2134, 2144-2148, 2499, 2860, 2976, 3010-3035, 3126-3134, 3287, 3361-3367, 3579-3580, 3587, 3590-3593, 3597, 3647, 3708, 3733-3757
R/tt_afun_utils.R              411      32  92.21%   48, 155, 162, 171-184, 250, 258-259, 477, 485-488, 570-574, 594, 608-610
R/tt_compare_tables.R           70       4  94.29%   51, 174, 246, 250
R/tt_compatibility.R           520      63  87.88%   19, 142-143, 186, 191, 319-320, 324-327, 333, 337, 388-391, 394-397, 519, 567, 600, 620, 653-656, 697, 714-718, 801, 828-831, 840, 902, 910, 921-924, 1035, 1042, 1070-1084, 1115-1116
R/tt_dotabulation.R           1125      96  91.47%   54, 246, 251, 253, 301, 325, 329-332, 364-367, 390, 423-426, 454-457, 552, 690-694, 743, 747, 775-778, 788, 808-812, 819-822, 1082, 1086, 1117, 1219-1222, 1425-1433, 1573, 1663-1672, 1754-1757, 1768, 1773, 1778-1779, 1781, 1792, 1797, 1820, 1906-1925
R/tt_export.R                  513      31  93.96%   43, 181-185, 233-236, 288-291, 436, 442, 474, 526, 806, 815, 840-844, 1010-1013, 1016, 1047, 1053
R/tt_from_df.R                  15       0  100.00%
R/tt_paginate.R                499      38  92.38%   40, 74, 122-131, 430, 547-550, 571-575, 720-723, 774-781, 858, 861, 879, 886, 889
R/tt_pos_and_access.R          571      43  92.47%   76, 78-80, 105, 166, 212-216, 258, 507, 509, 517, 523, 537, 547-550, 730, 741-745, 750-753, 780, 833-834, 845, 1007-1008, 1064-1092, 1361, 1436
R/tt_showmethods.R             144      21  85.42%   56, 91-113, 173, 199, 208, 213, 216-220, 309-310
R/tt_sort.R                    101       5  95.05%   243-246, 254
R/tt_toString.R                396      27  93.18%   123, 332-335, 341, 354, 364, 370, 373, 379-389, 475, 537, 543, 773-798
R/utils.R                       29       0  100.00%
R/validate_table_struct.R       84      10  88.10%   80-84, 93-94, 140, 149-150
R/Viewer.R                      61       9  85.25%   46, 50, 60-64, 84, 118
TOTAL                         8080     760  90.59%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  -------
R/00tabletrees.R             +5       0  +0.06%
R/colby_constructors.R       +7       0  +0.04%
R/make_split_fun.R          +19      +8  -3.14%
R/make_subset_expr.R         +7      +2  -0.89%
R/split_funs.R               +5       0  +0.13%
R/tree_accessors.R           +2      +1  -0.08%
R/tt_compatibility.R        +10      +7  -1.13%
R/tt_paginate.R             +59      +1  +0.79%
R/tt_toString.R              +7       0  +0.12%
TOTAL                      +121     +19  -0.10%

Results for commit: 733b723

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gmbecker
Copy link
Collaborator Author

All passing except spell check on Daniel's name which @edelarua says is a known bug @Melkiades @shajoezhu

Melkiades and others added 2 commits May 31, 2024 16:08
Co-authored-by: Joe Zhu <joe.zhu@roche.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
Copy link
Contributor

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

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

@shajoezhu @edelarua @ayogasekaram I think all PRs are good to go. Could you take a look again? If it is ok, I will approve them all today.

@gmbecker could you add more coverage tests to {formatters} please? only for toString file probably, it is doing -5% in coverage

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

@Melkiades this all looks good to me! Everything working as expected in my tests.

@shajoezhu
Copy link
Collaborator

lgtm! Thanks a lot guys!

Signed-off-by: Davide Garolini <dgarolini@gmail.com>
@Melkiades Melkiades merged commit 23dca85 into main Jun 4, 2024
@Melkiades Melkiades deleted the 261_truetype_pag branch June 4, 2024 13:21
@github-actions github-actions bot locked and limited conversation to collaborators Jun 4, 2024
@averissimo
Copy link
Contributor

averissimo commented Jun 7, 2024

I'm seeing a bunch of errors with fontspec from R CMD check on some simple PRs that only upgrade rmarkdown minimal package version.

The error appears from examples, tests and documentation generation.

I'm having trouble replicating locally on my host computer, as well as using the CI image (with staged.dependencies beforehand)

Example:

@gmbecker
Copy link
Collaborator Author

gmbecker commented Jun 7, 2024

@averissimo from what I can tell this is due to (somehow, not sure how) having a new (after the merge) version of formatters paired with an old version of rtables (the one direction of problematic pairing that versioned dependencies can't protect us from).

I'm not entirely sure how your jobs are choosing what to install from where, but if they use either a) formatters and rtables versions both from CRAN or b) formatters and rtables versions both from the respective main branches in the repos this error will go away

@Melkiades
Copy link
Contributor

Melkiades commented Jun 7, 2024

During last updates with TrueType PRs I tried to use the automatic versioning to set the deps but I think it is better to do as we are doing for the next PR in rtables and downstream deps. Eventually, there, the deps should be exact (we are talking one dev version of difference). I am adding a version to rtables connection to formatters there now (I doubt it is the source of the errors honestly though). See #876

@averissimo
Copy link
Contributor

averissimo commented Jun 10, 2024

Thanks for the comments, it will help to better pinpoint the problem

I have 2 big concerns with this problem:

  1. The current "Check" workflow in main branch of {teal.reporter} is failing
  2. This is showing in packages that have both formatters and rtables on staged.dependencies (upstream repos)
    • tern.mmrm

Edit: These are the latest dev versions (screenshot from teal.reporter@main check workflow)

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants