search

atusy / ftExtra

Helper functions for the flextable package
https://ftextra.atusy.net/
Other
65 stars 5 forks source link

Possible to optionally skip Pandoc testcase? #99

Open barracuda156 opened 9 months ago

barracuda156 commented 9 months ago

Tests fail to pass if Pandoc is not installed:

2024-02-29T07:44:27.2009130Z   Running ‘testthat.R’
2024-02-29T07:44:27.2059980Z  ERROR
2024-02-29T07:44:27.2063040Z Running the tests in ‘tests/testthat.R’ failed.
2024-02-29T07:44:27.2063380Z Last 13 lines of output:
2024-02-29T07:44:27.2064190Z   ══ Skipped tests (3) ═══════════════════════════════════════════════════════════
2024-02-29T07:44:27.2064710Z   • On CRAN (3): 'test-footnote.R:2:3', 'test-footnote.R:7:3',
2024-02-29T07:44:27.2065120Z     'test-footnote.R:17:3'
2024-02-29T07:44:27.2065320Z   
2024-02-29T07:44:27.2065660Z   ══ Failed tests ════════════════════════════════════════════════════════════════
2024-02-29T07:44:27.2066370Z   ── Error ('test-footnote.R:37:3'): generating footnote refs with callbacks ─────
2024-02-29T07:44:27.2066840Z   Error: pandoc document conversion failed with error 127
2024-02-29T07:44:27.2067150Z   Backtrace:
2024-02-29T07:44:27.2067310Z       ▆
2024-02-29T07:44:27.2067840Z    1. └─testthat::expect_snapshot(opt$ref(c(1L, 2L), "header", TRUE)) at test-footnote.R:37:3
2024-02-29T07:44:27.2068340Z    2.   └─rlang::cnd_signal(state$error)
2024-02-29T07:44:27.2068580Z   
2024-02-29T07:44:27.2068750Z   [ FAIL 1 | WARN 0 | SKIP 3 | PASS 43 ]
2024-02-29T07:44:27.2069070Z   Error: Test failures
2024-02-29T07:44:27.2069260Z   Execution halted
2024-02-29T07:44:27.3279870Z * checking for unstated dependencies in vignettes ... OK
2024-02-29T07:44:27.3292680Z * checking package vignettes in ‘inst/doc’ ... OK
2024-02-29T07:44:27.3293200Z * checking running R code from vignettes ...
2024-02-29T07:44:27.4671280Z   ‘format_columns.Rmd’ using ‘UTF-8’... OK
2024-02-29T07:44:27.6123530Z   ‘group-rows.Rmd’ using ‘UTF-8’... OK
2024-02-29T07:44:27.7574900Z   ‘transform-headers.Rmd’ using ‘UTF-8’... OK
2024-02-29T07:44:27.7575250Z  NONE
2024-02-29T07:44:27.7575550Z * checking re-building of vignette outputs ... SKIPPED
2024-02-29T07:44:27.7577330Z * DONE
2024-02-29T07:44:27.7579700Z 
2024-02-29T07:44:27.7579940Z Status: 1 ERROR

However, Pandoc is broken on some systems, and a non-trivial dependency to build. Is it possible to conditionally skip it or may it non-obligatory?

atusy commented 9 months ago

Is it your local environment? If so, I think Pandoc is mandatory for the development.

I have released v0.6.2 yesterday on CRAN, and it seems at least r-release-macos-x86_64 has passed the tests. Some have not yet been tested. If there are anything wrong on CRAN, I would add a workaround.

https://cran.r-project.org/web/checks/check_results_ftExtra.html

barracuda156 commented 9 months ago

@atusy Thank you for responding.

That was on GitHub CI, because I did not require Pandoc dependency. However we do not want to require it, for the reasons I mentioned: at the very least, it is just unavailable for some platforms where modern GHC and friends are broken. Everything else works fine, only Pandoc testcase fails.

It won’t show up on CRAN, since they, I guess, pre-install Pandoc for their checks.

This is not a bug, of course, but it makes tests fail unnecessarily, which otherwise would pass.

barracuda156 commented 9 months ago

For example, we skip rebuilding vignettes and manuals in order to avoid dependency on other heavy and unnecessary for R itself stuff like Tex. (Tex is not broken, but it is a huge thing to build.) If there was an option to pass to R check to avoid a need of Pandoc, that would perfectly solve the issue. It does not have to be a default option, so nothing will change for CRAN.

atusy commented 9 months ago

Why do you perform ftExtra's tests on somewhere else...? I mean repositories other than this repository or its forks.

barracuda156 commented 9 months ago

We have ftExtra in Macports :) And would like to support running tests.

atusy commented 9 months ago

Ah hah! Now I see.

testthat package has skip features. Maybe I can do something...

https://testthat.r-lib.org/articles/skipping.html.

I want to avoid unintentional skip of tests. That said, I won't skip tests if Pandoc is unavailable. One way is to check environmental variable. Does it meet your purpose?

barracuda156 commented 9 months ago

@atusy Basically anything which would allow tests to pass normally without Pandoc requirement would solve the problem. We can pass an argument or set an environment variable.

I do not suggest to change default behavior, just add an option to avoid Pandoc if a user or downstream distribution prefers.

atusy commented 9 months ago

I guess a quick and robust solution is adding setup-pandoc action to your GHA workflow like below.

https://github.com/atusy/ftExtra/blob/862c5028f5af66f19ca3be68129e3edfa6abb971/.github/workflows/R-CMD-check.yaml#L34

I can consider skip feature, but the release will be four weeks later because I released the latest on yesterday.

Avoid submitting multiple versions of the same package in a short period of time. CRAN prefers at most one submission per month. If you need to fix a major bug, be apologetic. https://r-pkgs.org/R-CMD-check.html#description

atusy commented 9 months ago

skip feature

There are two options, but I do not like both...

barracuda156 commented 9 months ago

@atusy The issue is not about hacking CI in fact: I am adding a fix now for R portgroup which would ensure, I think, that tests are not run unless enabled. And then disabling tests gonna work again, and CI will pass.

However, given that all tests but one pass without Pacdoc, I thought we could find a reasonable way to enable tests, but without requiring Pandoc. And if they are enabled, then a failure becomes a problem for CI.

It may not be worth fixing this issue, if that requires a substantial rewrite of the code or having trade-offs in terms of code quality. In the worst case, I can make a local patch to get rid of Pandoc stuff, or just keep tests disabled.

P. S. To make CI work with Pandoc, portfile just has to declare dependency on Pandoc, which will result in CI installing Pandoc prior to building and testing ftExtra. However, that is undesirable, since it will apply not only to CI, but to local installations, and Pandoc is not supported across all systems which Macports supports.

barracuda156 commented 9 months ago

@atusy As a quick fix to make updating ftExtra in Macports possible (since CI now force running tests): https://github.com/macports/macports-ports/pull/22837/commits/0a1af6fc846b2cd3ad11102fb63ddb9f2434b847 Combined with https://github.com/macports/macports-ports/pull/22837/commits/953a115916dd293ee4dfbe85da4570381d12c705 which fixes skipping test run with the current CI set-up.