vincentarelbundock / tinysnapshot

Snapshots for unit tests using the tinytest framework for `R`. Includes expectations to test base `R` and `ggplot2` plots as well as console output from `print()`.
12 stars 0 forks source link

Duplicated snapshot content in function with `print()` #14

Open etiennebacher opened 6 months ago

etiennebacher commented 6 months ago

Hi Vincent, I'd like to use snapshot tests on some print output in tidypolars, but I run into some issues with tinysnapshot.

I made a mini test package here: https://github.com/etiennebacher/testtinysnapshot

To reproduce:

The snapshot has twice [1] "Hello, world!\n Hello again" while it should be printed only once. This makes all subsequent calls to tinytest::run_test_file("inst/tinytest/test-print.R") fail.

Can you reproduce or did I miss something?

vincentarelbundock commented 6 months ago

expect_snapshot_print() is designed to compare the output of the print() method associated with the returned object. It calls print() internally here on the object: https://github.com/vincentarelbundock/tinysnapshot/blob/main/R/expect_snapshot_print.R#L40

Since you're doing it already in the function, you get duplicated output.

You example works if you call return() instead of print() in your function.

etiennebacher commented 6 months ago

Ah I see, it might not be what I want then, thanks for the explanation

vincentarelbundock commented 6 months ago

Usually, I'd say that best practice is to avoid calling print() in a function, and to have proper methods. So in the vast majority of cases this should be a problem.

But if you describe your problem, maybe we can find a workaround

etiennebacher commented 6 months ago

Basically I'm adding a show_query() function in tidypolars that would show the "pure" polars code to reproduce the result, and I would like this to be printed in the console the same way it would appear in someone's script (without quotation marks, or [1], or other noise). See the first post here: https://github.com/etiennebacher/tidypolars/pull/103

To do that I use cat(), but then expect_snapshot_print() adds a NULL at the end because it tries to print() something as you explain, but there's nothing to print. I know that testthat::expect_snapshot() allows to capture what is shown in the console as-is, such as the output of cat(), but it seems tinysnapshot is slightly different in that regard.

The issue regarding duplicated output was only a by-product of me trying to make a small reprex.

vincentarelbundock commented 6 months ago

Right. I would argue that the "standards-compliant" way of dealing with this would be for show_query() to return an object of class "tidypolars_query", and then to define a print.tidypolars_query() which basically does only cat().

But maybe you're right that tinysnapshot should be more flexible about what input formats it allows. I'd have to think about how to implement this (and time is scarce).

etiennebacher commented 6 months ago

Right. I would argue that the "standards-compliant" way of dealing with this would be for show_query() to return an object of class "tidypolars_query", and then to define a print.tidypolars_query() which basically does only cat().

I think it's a good solution, thanks. I implemented this in https://github.com/etiennebacher/tidypolars/pull/103/commits/121c97f70d127791d660bc939a6963f704a915b8 and it seems to work. I'll explore more extensively later.

I'd have to think about how to implement this (and time is scarce).

This issue might provide a workaround for others that run into this need, you don't necessarily need to change the package.

vincentarelbundock commented 6 months ago

Cool. Will leave it open and label wontfix for now.