Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible to optionally skip Pandoc testcase? #99

Open
barracuda156 opened this issue Feb 29, 2024 · 11 comments
Open

Possible to optionally skip Pandoc testcase? #99

barracuda156 opened this issue Feb 29, 2024 · 11 comments

Comments

@barracuda156
Copy link

barracuda156 commented Feb 29, 2024

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?

@barracuda156 barracuda156 changed the title [macOS] Pandoc testcase fails Possible to optionally skip Pandoc testcase? Feb 29, 2024
@atusy
Copy link
Owner

atusy commented Feb 29, 2024

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Owner

atusy commented Feb 29, 2024

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

@barracuda156
Copy link
Author

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

@atusy
Copy link
Owner

atusy commented Feb 29, 2024

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
Copy link
Author

@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
Copy link
Owner

atusy commented Feb 29, 2024

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

- uses: r-lib/actions/setup-pandoc@v2

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
Copy link
Owner

atusy commented Feb 29, 2024

skip feature

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

@barracuda156
Copy link
Author

barracuda156 commented Feb 29, 2024

@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
Copy link
Author

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

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

No branches or pull requests

2 participants