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

expect_snapshot_value() fails when one attribute contains a function #1771

Closed
DanChaltiel opened this issue Apr 10, 2023 · 6 comments · Fixed by #1867
Closed

expect_snapshot_value() fails when one attribute contains a function #1771

DanChaltiel opened this issue Apr 10, 2023 · 6 comments · Fixed by #1867
Labels
feature a feature request or enhancement snapshot 📷
Milestone

Comments

@DanChaltiel
Copy link

Hi,

Here is a reprex:

test_that("matrix", {
  set.seed(42)
  x = 5
  attr(x, "f") = function(x) x
  expect_snapshot_value(x, style="json2")
})

Here, the only style that works is serialize, but in my real-life case, it generated a 9.7MB .md file that is totally unusable.

The error message does not inform that the cause of the failure is a function:

Error in check_roundtrip(value, load(value_enc), ..., tolerance = tolerance): Serialization round-trip is not symmetric.
environment(attr(value, 'f')) is env:0x000000003084be50
environment(attr(roundtrip, 'f')) is env:global
i You may need to consider serialization style

I think in this case the function should be ignored and a warning would be enough.

@hadley
Copy link
Member

hadley commented Jul 26, 2023

This is very much the nature of expect_snapshot_value(), and I believe if you are attempting to serialize an R object to a text string, it's your responsibility to figure out a good serialization, if the default doens't work for you.

@hadley hadley closed this as completed Jul 26, 2023
@DanChaltiel
Copy link
Author

@hadley I get your point, but still, the error message is very cryptic and it took me a lot of trials-errors to figure out that the problem was I was trying to serialize a function.

I am trying to make tests using this very nice package, but unfortunately, that doesn't make me a serialization expert, so I have no idea what is serializable and not.

A quick check inside expect_snapshot_value() with a custom error message "Cannot serialize functions with method XXX" would have been very nice.

@hadley
Copy link
Member

hadley commented Jul 27, 2023

I don't see how I could make it less confusing — it tells you that the serialization round-trip failed, and exactly what the difference was, i.e. environment(attr(value, 'f')) was different.

@hadley
Copy link
Member

hadley commented Jul 27, 2023

Why are you trying to snapshot the value of a function in this was in the first place?

@DanChaltiel
Copy link
Author

I was trying to snapshot the result of a function that, for obscure reasons, has a function as an attribute.
I am not the creator of the original function, but I was assigned the task of writing some tests for it.

I understand the concept of serialization, but the implementation and the limits are beyond my reach. The concept of "round-trip" is still obscure to me, despite my googling.
I suppose it's a bit hard for you to relate, as you're a very skilled programmer.

I now at least understand that it is not possible to serialize a function at all.
And I guess it is because the environment would be different during serialization and deserialization. But that's not obvious at all for someone like me.
I would thus expect expect_snapshot_value() to check that the object is serializable, i.e. doesn't contain a function.

In my example this would mean:

is_serializable = function(x) !is_function(x)
map_lgl(x, is_serializable) #Ok
map_lgl(attributes(x), is_serializable) #not ok, abort()

Of course, the check would need to be recursive, that's only for the example.

This way, there would be a clear message telling me that the problem was that the attribute f is a function and that functions are not serializable.
After that, I could easily remove the function manually, but an option ignore_not_serializable would be great!

This would allow programmers to use expect_snapshot_value() without knowing as much about serialization.

@hadley
Copy link
Member

hadley commented Jul 27, 2023

Ok, first off, I don't think that this is a good candidate for a snapshot test — you should be generally be using these for simple objects that have a clear textual representation, and in general functions aren't a good candidate for this (since they have environments etc). That said, it's definitely possible to serialize a function but in general serializing it to JSON is going to be hard, because JSON isn't rich enough to represent all of R's data types well.

I can try and make the error a bit more clear. Currently in your case it is:

Error in `check_roundtrip(value, load(value_enc), ..., tolerance = tolerance)`: Serialization round-trip is not symmetric.

`environment(attr(value, 'f'))` is <env:0x164a6ef60>
`environment(attr(roundtrip, 'f'))` is <env:global>

i You may need to consider serialization `style`

And it could maybe be:

Error: Object could not be safely serialized:
* Serializing and deserialzing the object has returned different results:

`environment(attr(value, 'f'))` is <env:0x164a6ef60>
`environment(attr(roundtrip, 'f'))` is <env:global>

i You may need to try a diffferent serialization `style`

@hadley hadley reopened this Jul 27, 2023
@hadley hadley added feature a feature request or enhancement snapshot 📷 labels Jul 27, 2023
@hadley hadley added this to the v3.2.0 milestone Sep 22, 2023
hadley added a commit that referenced this issue Sep 22, 2023
hadley added a commit that referenced this issue Sep 27, 2023
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 snapshot 📷
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants