dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

BinaryFormatterTestData.cs's mentioned "UpdateTestData" doesn't exist #104205

Open stephentoub opened 3 months ago

stephentoub commented 3 months ago

The BinaryFormatterTestData mentions an "UpdateTestData" test that should be enabled to update the data in the tests: https://github.com/dotnet/runtime/blob/9a4329d65c4d272e26b2c693341ef4b26a5bc8c8/src/libraries/System.Resources.Extensions/tests/BinaryFormatTests/Legacy/BinaryFormatterTestData.cs#L70-L86

But "UpdateTestData" doesn't exist.

danmoseley commented 3 months ago

IIRC it was in the code there so I expect git log -G"UpdateTestData will find out how it got deleted.

adamsitnik commented 2 months ago

IIRC it was in the code there so I expect git log -G"UpdateTestData will find out how it got deleted.

@danmoseley Thanks for sharing, it has allowed me to find the answer.

The full story:

  1. The original BinaryFormatter tests mention UpdateBlobs https://github.com/dotnet/runtime/blob/0f05719bd26563383d826f3dcbde53879baad58f/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs#L86
  2. It's was a test with commented out "[Fact]" attribute that was removed by Viktor in 2019 in https://github.com/dotnet/corefx/pull/38676

        // Used for updating blobs in BinaryFormatterTestData.cs
        //[Fact]
        public void UpdateBlobs()
        {
            string testDataFilePath = GetTestDataFilePath();
            string[] coreTypeBlobs = SerializableEqualityComparers_MemberData()
                .Concat(SerializableObjects_MemberData())
                .Select(record => BinaryFormatterHelpers.ToBase64String(record[0]))
                .ToArray();
    
            var (numberOfBlobs, numberOfFoundBlobs, numberOfUpdatedBlobs) = UpdateCoreTypeBlobs(testDataFilePath, coreTypeBlobs);
            Console.WriteLine($"{numberOfBlobs} existing blobs" +
                $"{Environment.NewLine}{numberOfFoundBlobs} found blobs with regex search" +
                $"{Environment.NewLine}{numberOfUpdatedBlobs} updated blobs with regex replace");
        }
  3. When Jeremy copied this code to WinForms, he has renamed it to UpdateTestData and introduced a new implementation.
    [Fact(Skip = "Only used to update test data.")]
    public void UpdateTestData()
    {
        List<string> serializedHashes = [];
        foreach ((object obj, _) in SerializableEqualityComparers().Concat(SerializableObjects()))
        {
            BinaryFormatter bf = new();
            using MemoryStream ms = new();
            bf.Serialize(ms, obj);
            string serializedHash = Convert.ToBase64String(ms.ToArray());
            serializedHashes.Add(serializedHash);
        }

        string path = @"..\..\..\..\..\src\System.Private.Windows.Core\tests\BinaryFormatTests\FormatTests\Legacy\BinaryFormatterTestData.cs";
        string[] blobs = [.. serializedHashes];

        UpdateCoreTypeBlobs(path, blobs);
    }
  1. When I was moving the code back to dotnet/runtime in https://github.com/dotnet/runtime/pull/102379 I've simply not ported this method as it had too many things hardcoded.

Since updating this comment does not seem to be urgent, I am going to change the labeling and move this issue to future.

Whoever wants to restore these two methods should most likely start with de-duplicating https://github.com/dotnet/runtime/blob/main/src/libraries/System.Resources.Extensions/tests/BinaryFormatTests/Legacy/BinaryFormatterTestData.cs and https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs and then re-introducing one of them and updating the comment(s) to point to the new name.

stephentoub commented 2 months ago

Since updating this comment does not seem to be urgent

Not urgent, but fwiw the comment isn't the thing that blocked me but rather the lack of the code that actually did the update, as I needed to update the test data. I worked around it, though.