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

Junit reporter does not use standard decimal separator #1660

Closed
maksymiuks opened this issue Jul 21, 2022 · 1 comment · Fixed by #1885
Closed

Junit reporter does not use standard decimal separator #1660

maksymiuks opened this issue Jul 21, 2022 · 1 comment · Fixed by #1885
Labels
feature a feature request or enhancement reporter 📝

Comments

@maksymiuks
Copy link
Contributor

Hi,

I identified the following issue while trying to read back a junit file created for ggplot2 test suite. In this package, one of the tests is executed with a different option for decimal separator (https://github.com/tidyverse/ggplot2/blob/main/tests/testthat/test-aes.r#L19-L25). Therefore when testthat collects junit data it uses an altered option and due to the as.character() time is saved using a comma as separator (

xml2::xml_attr(self$suite, "time") <- as.character(round(self$suite_time, 3))
). Because of that, I'm receiving coercing error while trying to read it back.

In the case of the come, the issue is not that huge, but it sep option can be set to an arbitrary sign rendering the time field useless to read back for someone unaware. As the issue comes from the as.character(), I suggested the following change

xml2::xml_attr(self$suite, "time") <- withr::with_options(list(OutDec = "."), as.character(round(self$suite_time, 3)))

thanks to that we won't compromise any test environment but at the same time always get numbers in a standard format recognizable by any system - with point as decimal separator. I've checked and withr is already a dependency so it won't add any new.

Let me know what you think about this suggestion, I'm happy to prepare a PR if you agree with my evaluation and approach.

@hadley hadley added feature a feature request or enhancement reporter 📝 labels Sep 19, 2022
@hadley
Copy link
Member

hadley commented Sep 19, 2022

A PR would be great. I think you could probably use withr::local_options() scoped to the full function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement reporter 📝
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants