Enabling Scalar Tests #122
Conversation
| @@ -2132,3 +2128,11 @@ func TestEngineRecoversFromPanic(t *testing.T) { | |||
| }) | |||
|
|
|||
| } | |||
|
|
|||
| func jsonEqual(t *testing.T, expected interface{}, actual interface{}) { | |||
| e, err := json.Marshal(expected) | |||
There was a problem hiding this comment.
I think this could lead to more problems while checking equality. JSON doesn't support NaN or Inf (golang/go#3480). There's also stuff with ordering (golang/go#6244) that could end up as confusing bugs, as labels are marshaled as a map.
Maybe not handling all those complications here and just comparing structs recursively would be better? I raised efficientgo/core#3 a few days ago to add support for NaN. I'll ping @bwplotka for review, and maybe that can be used here instead? WDYT? 🙂
There was a problem hiding this comment.
If we are blocked by that PR, maybe there's a way to add these cases in a separate test and use a custom comparison function?
There was a problem hiding this comment.
Now that efficientgo/core#3 is merged, can we enable the tests for NaN values?
There was a problem hiding this comment.
Couldnt we just compare oldResult.String() and newResult.String() for string equality?
Comparing json allow us to enable those tests.
I wanted to do a change on scalar and having those tests enabled give us more confidence to do the change.