From 04f3e2389b4e194793c2a2c941ad64db31662ad0 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Tue, 30 Jul 2024 15:48:26 -0500 Subject: [PATCH 1/4] Telemetry strings could be null, so handle that case --- src/Framework/TelemetryEventArgs.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Framework/TelemetryEventArgs.cs b/src/Framework/TelemetryEventArgs.cs index 276ec0a1fa3..37fdcf0589c 100644 --- a/src/Framework/TelemetryEventArgs.cs +++ b/src/Framework/TelemetryEventArgs.cs @@ -37,10 +37,9 @@ internal override void WriteToStream(BinaryWriter writer) foreach (var kvp in Properties) { writer.Write(kvp.Key); - writer.Write(kvp.Value); + writer.WriteOptionalString(kvp.Value); } } - internal override void CreateFromStream(BinaryReader reader, int version) { base.CreateFromStream(reader, version); @@ -51,7 +50,7 @@ internal override void CreateFromStream(BinaryReader reader, int version) for (int i = 0; i < count; i++) { string key = reader.ReadString(); - string value = reader.ReadString(); + string value = reader.ReadOptionalString(); Properties.Add(key, value); } } From f4b90a085bbb8817386de98e9db36cb6c40b1220 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Tue, 30 Jul 2024 16:17:36 -0500 Subject: [PATCH 2/4] Bump version --- eng/Versions.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/Versions.props b/eng/Versions.props index c8d6176ba64..1fa71c720eb 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -2,7 +2,7 @@ - 17.11.3release + 17.11.4release 17.10.4 15.1.0.0 preview From 036459bfdb74abae3b2f408539568082704a3cf1 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Tue, 30 Jul 2024 16:24:41 -0500 Subject: [PATCH 3/4] Add tests to cover TelemetryEventArg round-trip serialization --- .../BuildEventArgsSerialization_Tests.cs | 1 + .../CustomEventArgSerialization_Tests.cs | 57 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs index 656f906ccfa..2470064694c 100644 --- a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs +++ b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs @@ -873,6 +873,7 @@ public void PropertyInitialValueEventArgs() e => e.HelpKeyword, e => e.SenderName); } + [Fact] public void ReadingCorruptedStreamThrows() { diff --git a/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs b/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs index 5e65f74a575..4be84f14473 100644 --- a/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs +++ b/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs @@ -974,5 +974,62 @@ private static void VerifyTaskFinished(TaskFinishedEventArgs genericEvent, TaskF newGenericEvent.TaskFile.ShouldBe(genericEvent.TaskFile, StringCompareShould.IgnoreCase); // "Expected TaskFile to Match" newGenericEvent.TaskName.ShouldBe(genericEvent.TaskName, StringCompareShould.IgnoreCase); // "Expected TaskName to Match" } + + + [Fact] + public void TestTelemetryEventArgs() + { + // Test using reasonable values + TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary { { "Key", "Value" } } }; + genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2); + + // Serialize + genericEvent.WriteToStream(_writer); + long streamWriteEndPosition = _stream.Position; + + // Deserialize and Verify + _stream.Position = 0; + TelemetryEventArgs newGenericEvent = new TelemetryEventArgs(); + newGenericEvent.CreateFromStream(_reader, _eventArgVersion); + _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" + VerifyGenericEventArg(genericEvent, newGenericEvent); + VerifyTelemetryEvent(genericEvent, newGenericEvent); + + // Test using null event name + _stream.Position = 0; + genericEvent = new TelemetryEventArgs { EventName = null, Properties = new Dictionary { { "Key", "Value" } } }; + // Serialize + genericEvent.WriteToStream(_writer); + streamWriteEndPosition = _stream.Position; + + // Deserialize and Verify + _stream.Position = 0; + newGenericEvent = new TelemetryEventArgs(); + newGenericEvent.CreateFromStream(_reader, _eventArgVersion); + _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" + VerifyGenericEventArg(genericEvent, newGenericEvent); + VerifyTelemetryEvent(genericEvent, newGenericEvent); + + // Test using null property value name + _stream.Position = 0; + genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary { { "Key", null } } }; + // Serialize + genericEvent.WriteToStream(_writer); + streamWriteEndPosition = _stream.Position; + + // Deserialize and Verify + _stream.Position = 0; + newGenericEvent = new TelemetryEventArgs(); + newGenericEvent.CreateFromStream(_reader, _eventArgVersion); + _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" + VerifyGenericEventArg(genericEvent, newGenericEvent); + VerifyTelemetryEvent(genericEvent, newGenericEvent); + } + + private static void VerifyTelemetryEvent(TelemetryEventArgs expected, TelemetryEventArgs actual) + { + actual.EventName.ShouldBe(expected.EventName); + actual.Properties.ShouldBe(expected.Properties); + } } } From 7201430b76f60d4b36436bf28bfc1907214a6747 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Wed, 31 Jul 2024 09:07:07 -0500 Subject: [PATCH 4/4] Test cleanups and nullability in the TelemetryEventargs --- .../CustomEventArgSerialization_Tests.cs | 76 ++++++++++++------- src/Framework/TelemetryEventArgs.cs | 13 ++-- 2 files changed, 56 insertions(+), 33 deletions(-) diff --git a/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs b/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs index 4be84f14473..0da144267c5 100644 --- a/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs +++ b/src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs @@ -977,53 +977,73 @@ private static void VerifyTaskFinished(TaskFinishedEventArgs genericEvent, TaskF [Fact] - public void TestTelemetryEventArgs() + public void TestTelemetryEventArgs_AllProperties() { // Test using reasonable values TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary { { "Key", "Value" } } }; genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2); - // Serialize - genericEvent.WriteToStream(_writer); - long streamWriteEndPosition = _stream.Position; + TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent); - // Deserialize and Verify - _stream.Position = 0; - TelemetryEventArgs newGenericEvent = new TelemetryEventArgs(); - newGenericEvent.CreateFromStream(_reader, _eventArgVersion); - _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" VerifyGenericEventArg(genericEvent, newGenericEvent); VerifyTelemetryEvent(genericEvent, newGenericEvent); + } + + [Fact] + public void TestTelemetryEventArgs_NullProperties() + { + // Test using reasonable values + TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = null }; + genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2); + TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent); + + // quirk - the properties dict is initialized to an empty dictionary by the default constructor, so it's not _really_ round-trippable. + // so we modify the source event for easier comparison here. + genericEvent.Properties = new Dictionary(); + + VerifyGenericEventArg(genericEvent, newGenericEvent); + VerifyTelemetryEvent(genericEvent, newGenericEvent); + } + + [Fact] + public void TestTelemetryEventArgs_NullEventName() + { // Test using null event name - _stream.Position = 0; - genericEvent = new TelemetryEventArgs { EventName = null, Properties = new Dictionary { { "Key", "Value" } } }; - // Serialize - genericEvent.WriteToStream(_writer); - streamWriteEndPosition = _stream.Position; + TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = null, Properties = new Dictionary { { "Key", "Value" } } }; + genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2); + + TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent); - // Deserialize and Verify - _stream.Position = 0; - newGenericEvent = new TelemetryEventArgs(); - newGenericEvent.CreateFromStream(_reader, _eventArgVersion); - _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" VerifyGenericEventArg(genericEvent, newGenericEvent); VerifyTelemetryEvent(genericEvent, newGenericEvent); + } + [Fact] + public void TestTelemetryEventArgs_NullPropertyValue() + { // Test using null property value name + TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary { { "Key", null } } }; + genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2); + + TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent); + + VerifyGenericEventArg(genericEvent, newGenericEvent); + VerifyTelemetryEvent(genericEvent, newGenericEvent); + } + + private T RoundTrip(T original) + where T : BuildEventArgs, new() + { _stream.Position = 0; - genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary { { "Key", null } } }; - // Serialize - genericEvent.WriteToStream(_writer); - streamWriteEndPosition = _stream.Position; + original.WriteToStream(_writer); + long streamWriteEndPosition = _stream.Position; - // Deserialize and Verify _stream.Position = 0; - newGenericEvent = new TelemetryEventArgs(); - newGenericEvent.CreateFromStream(_reader, _eventArgVersion); + var actual = new T(); + actual.CreateFromStream(_reader, _eventArgVersion); _stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match" - VerifyGenericEventArg(genericEvent, newGenericEvent); - VerifyTelemetryEvent(genericEvent, newGenericEvent); + return actual; } private static void VerifyTelemetryEvent(TelemetryEventArgs expected, TelemetryEventArgs actual) diff --git a/src/Framework/TelemetryEventArgs.cs b/src/Framework/TelemetryEventArgs.cs index 37fdcf0589c..d3d57e9c5e5 100644 --- a/src/Framework/TelemetryEventArgs.cs +++ b/src/Framework/TelemetryEventArgs.cs @@ -6,8 +6,6 @@ using System.IO; using Microsoft.Build.Shared; -#nullable disable - namespace Microsoft.Build.Framework { /// @@ -19,12 +17,12 @@ public sealed class TelemetryEventArgs : BuildEventArgs /// /// Gets or sets the name of the event. /// - public string EventName { get; set; } + public string? EventName { get; set; } /// /// Gets or sets a list of properties associated with the event. /// - public IDictionary Properties { get; set; } = new Dictionary(); + public IDictionary Properties { get; set; } = new Dictionary(); internal override void WriteToStream(BinaryWriter writer) { @@ -34,6 +32,11 @@ internal override void WriteToStream(BinaryWriter writer) int count = Properties?.Count ?? 0; writer.Write7BitEncodedInt(count); + if (Properties == null) + { + return; + } + foreach (var kvp in Properties) { writer.Write(kvp.Key); @@ -50,7 +53,7 @@ internal override void CreateFromStream(BinaryReader reader, int version) for (int i = 0; i < count; i++) { string key = reader.ReadString(); - string value = reader.ReadOptionalString(); + string? value = reader.ReadOptionalString(); Properties.Add(key, value); } }