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

Enhance property bag testing #1673

Closed
ghost opened this issue Sep 13, 2019 · 4 comments
Closed

Enhance property bag testing #1673

ghost opened this issue Sep 13, 2019 · 4 comments
Labels
area-test Issues for improving the tests and adding new ones. bug enhancement resolved-fixed

Comments

@ghost
Copy link

ghost commented Sep 13, 2019

Per @michaelcfanning:

Add tests for the range of types that might occur in a property bag. For example, consider serializing a double that exceeds Int32.Max. Pass a float that is not an integral value. Consider round-tripping a simple custom object. And serialize/deserialize arrays of every supported thing. Understand and test expected behaviors when setting property values to null as well.

@ghost ghost added enhancement area-test Issues for improving the tests and adding new ones. labels Sep 13, 2019
@ScottLouvau
Copy link
Collaborator

Roundtripping DateTimes doesn't work today, with or without my change to remove the extra quotes from SerializedPropertyInfoConverter.WriteJson.

It seems SerializedPropertyInfoConverter gets to write the value, but PropertyBagConverter reads the value back, and JsonSerializer.Populate is messing with the DateTime in that case.

@ghost ghost self-assigned this Sep 19, 2019
@ghost ghost added the bug label Sep 19, 2019
@ghost
Copy link
Author

ghost commented Sep 26, 2019

@ScottLouvau Roundtripping date/times does work in certain scenarios. PR #1651, which fixed issue #1577, demonstrates one such scenario (going through the PrereleaseCompatibilityTransformer), and I have a little test program that demonstrates another one (simple deserialize/re-serialize from a SARIF file).

OTOH I absolutely believe you've seen a real bug, and that it's related to your observation that serialization and deserialization go through different code paths. Do you have a repro that I could put in a failing unit test?

@ghost
Copy link
Author

ghost commented Sep 26, 2019

Test program:

using System.IO;
using FluentAssertions;
using Microsoft.CodeAnalysis.Sarif;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace Bug1673
{
    class Program
    {
        static void Main(string[] args)
        {
            string logContents = File.ReadAllText(@"TestData\input.sarif");

            SarifLog log = JsonConvert.DeserializeObject<SarifLog>(logContents);
            string roundTrippedLogContents = JsonConvert.SerializeObject(log, Formatting.Indented);

            roundTrippedLogContents.Should().Be(logContents);

            JObject jObject = JsonConvert.DeserializeObject<JObject>(logContents);
            roundTrippedLogContents = JsonConvert.SerializeObject(jObject, Formatting.Indented);

            roundTrippedLogContents.Should().Be(logContents);
        }
    }
}

Input:

{
  "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.4.json",
  "version": "2.1.0",
  "runs": [
    {
      "tool": {
        "driver": {
          "name": "Bug1673Tester"
        }
      },
      "results": [
        {
          "ruleId": "TST0001",
          "message": {
            "text": "Hello."
          },
          "properties": {
            "date": "2019-09-25T15:07"
          }
        }
      ],
      "columnKind": "utf16CodeUnits"
    }
  ]
}

@ScottLouvau
Copy link
Collaborator

Yes. Re-add DateTime roundtrip to PropertyBagConverter_RoundTripsStringPropertyWithEscapedCharacters().

This failed when I ran it, but I realized I couldn't fix it.
See b323c58#diff-802ec850c9c3388f7307ab9e42c67453L28 for the code to re-add.

@ghost ghost closed this as completed in 68182f0 Sep 30, 2019
@ghost ghost added the resolved-fixed label Sep 30, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-test Issues for improving the tests and adding new ones. bug enhancement resolved-fixed
Projects
None yet
Development

No branches or pull requests

1 participant