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

test: Update test snapshot #10318

Closed
wants to merge 1 commit into from
Closed

test: Update test snapshot #10318

wants to merge 1 commit into from

Conversation

yufeih
Copy link
Contributor

@yufeih yufeih commented Oct 23, 2024

No description provided.

@yufeih
Copy link
Contributor Author

yufeih commented Oct 23, 2024

Snapshot testing seems broken

@filzrev
Copy link
Contributor

filzrev commented Oct 24, 2024

These diffs are caused by changing JSON serialize from NewtonsoftJson to System.Text.Json.

var value = 1.0d;
Console.WriteLine(System.Text.Json.JsonSerializer.Serialize(value));   // Output: `1`
Console.WriteLine(Newtonsoft.Json.JsonConvert.SerializeObject(value)); // Output: `1.0`

Both result are correct JSON representation.
Though it might be better to serialize as 1.0. (by adding custom JSON converter)

Because when JSON deserialized as Dictionary<string, object>.
Deserialized value type is inferred based on number representation.

case JsonTokenType.Number when reader.TryGetInt32(out int intValue):
return intValue;
case JsonTokenType.Number when reader.TryGetInt64(out long longValue):
return longValue;
case JsonTokenType.Number:
return reader.GetDouble();

@yufeih
Copy link
Contributor Author

yufeih commented Oct 24, 2024

I understand where the diff is coming from, what I'm trying to figure out is why these diffs didn't fail the PRs that introduced them.

@filzrev
Copy link
Contributor

filzrev commented Oct 24, 2024

As comment at #10156
Currently CI don't validate snapshot files because SNAPSHOT_TEST is not set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants