Junit reporter and crayon

[maksymiuks]

Hi

recently when running rlang's testing suite, I discovered an issue when combining Junit reporter, test_that_cli() and tests skip. To illustrate it, I'll use one of rlang's unit tests as an example https://github.com/r-lib/rlang/blob/c55f6027928d3104ed449e591e8a225fcaf55e13/tests/testthat/test-cnd-message.R#L200-L203

Let's create a Junit reporter such that crayon and unicode will be disabled

jreporter <- withr::with_options(list(
  width = 80L,
  testthat.progress.max_fails = 1L,
  testthat.use_colours = FALSE,
  cli.unicode = FALSE,
  crayon.enabled = FALSE,
  useFancyQuotes = FALSE
), {
  testthat::JunitReporter$new(file = "./junit.xml")
})
> jreporter$crayon
[1] FALSE
> jreporter$unicode
[1] FALSE

we don't want to include any special signs as XML breaks if the ESC sign is included as part of it. It cannot be read back due to the invalid character in the attribute value error. Additionally, the content of this XML could later be used to feed the md table for instance and pandoc also breaks on the ESC sign. I expect that if I set reporter settings to FALSE for both crayon and unicode, such signs won't appear in the output regardless of settings for particular tests, ie. tests can use these but the reporter will strip them out from the output at the end. Unfortunately, that's not what happens - test_that_cli() overwrites test options https://github.com/r-lib/cli/blob/main/R/test.R#L86-L89 and unfortunately, they end up in the output leading to invalid XML. The aformentioned rlang example https://github.com/r-lib/rlang/blob/c55f6027928d3104ed449e591e8a225fcaf55e13/tests/testthat/test-cnd-message.R#L200-L203 will produce following junit file

<?xml version="1.0" encoding="UTF-8"?>                                                                                                                            
<testsuites>                                                                                                                                                          
  <testsuite name="cnd-message" timestamp="2023-09-12T11:45:22Z" hostname="c54242a49011" tests="68" skipped="20" failures="0" errors="0" time="22.66">
<testcase time="0.258999999999958" classname="cnd_message" name="format_error_bullets_generates_bullets_plain_">                                            
<skipped message="Reason: On CRAN ('test-cnd-message.R:201:3')"/>                                                                                             
</testcase>                                                                                                                                                       
    <testcase time="0.245000000000005" classname="cnd_message" name="format_error_bullets_generates_bullets_ansi_">                                             
<skipped message="Reason: On CRAN (\033[34mtest-cnd-message.R:201:3\033[39m)"/>                                                                               
</testcase>                                                                                                                                                       
    <testcase time="0.242000000000019" classname="cnd_message" name="format_error_bullets_generates_bullets_unicode_">                                          
<skipped message="Reason: On CRAN ('test-cnd-message.R:201:3')"/>                                                                                             
</testcase>                                                                                                                                                       
    <testcase time="0.240000000000009" classname="cnd_message" name="format_error_bullets_generates_bullets_fancy_">                                            
<skipped message="Reason: On CRAN (\033[34mtest-cnd-message.R:201:3\033[39m)"/>                                                                               
</testcase>                                                                                                                                                       
  </testsuite>                                                                                                                                                        
</testsuites>

which could not be read back due to the ESC character (here represented by \033)

> xml2::read_xml("junit.xml")
Error in read_xml.character("junit.xml") : 
  invalid character in attribute value [9]

I think that a junit reporter, which is designed to generate .xml files, should ensure that the generated file is valid.
My suggestion is that regardless of particular test outputs if the reporter's crayon and unicode are set to true, the reporter should post-process to the file to remove any undesired content.

As a side note, I'll add that the issue becomes apparent only when skipping tests (CRAN in this case). If tests are passing, no undesired signs sneak through to the junit file.

I hope the explanation is clear, but if you need further examples, let me know, and I will prepare a dummy package showcasing this problem.

[maksymiuks]

Hi

I see it was marked as a bug. May I ask how do you imagine it could be fixed? I'm happy to prepare a PR fixing the issue but I don't know what your overall idea is about the course this should take.

Personally, I thought about a brute force, namely if private fields cli.unicode and crayon.enabled are both false, all data that is saved to the file is treated with cli::ansi_strip ensuring that xml is valid. I appreciate it's not really elegant but at the same time, it feels like something that we want to have.

Either way, I'm happy to hear you overall plan so I can prepare a PR.