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.

Sign up for GitHub

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

Jump to bottom

Enable nullable for EventLogCollector #3674

Merged
merged 3 commits into from
May 27, 2022
Merged

Enable nullable for EventLogCollector #3674

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8547920
Enable nullable for EventLogCollector
Evangelink May 24, 2022
1d7c4c6
Update code
Evangelink May 24, 2022
c49681c
Fix warnings
Evangelink May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions ...osoft.TestPlatform.Extensions.EventLogCollector/CollectorNameValueConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

using Microsoft.VisualStudio.TestPlatform.ObjectModel;

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
Expand All @@ -32,7 +30,7 @@ internal class CollectorNameValueConfigurationManager
/// <param name="configurationElement">
/// XML element containing the configuration
/// </param>
public CollectorNameValueConfigurationManager(XmlElement configurationElement)
public CollectorNameValueConfigurationManager(XmlElement? configurationElement)
{
if (configurationElement == null)
{
Expand Down Expand Up @@ -79,14 +77,14 @@ public CollectorNameValueConfigurationManager(XmlElement configurationElement)
}
}

internal IDictionary<string, string> NameValuePairs { get; } = new Dictionary<string, string>();
internal IDictionary<string, string?> NameValuePairs { get; } = new Dictionary<string, string?>();

/// <summary>
/// Gets the value of the setting specified by name, or null if it was not found
/// </summary>
/// <param name="name">The setting name</param>
/// <returns>The setting value, or null if the setting was not found</returns>
public string this[string name]
public string? this[string name]
{
get
{
Expand Down
4 changes: 1 addition & 3 deletions ...lectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogCollectorException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

using System;

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
Expand All @@ -17,7 +15,7 @@ internal class EventLogCollectorException : Exception
/// </summary>
/// <param name="localizedMessage">the localized exception message</param>
/// <param name="innerException">the inner exception</param>
public EventLogCollectorException(string localizedMessage, Exception innerException)
public EventLogCollectorException(string? localizedMessage, Exception? innerException)
: base(localizedMessage, innerException)
{
}
Expand Down
2 changes: 0 additions & 2 deletions src/DataCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
Expand Down
10 changes: 4 additions & 6 deletions src/DataCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,21 @@

using Resource = Microsoft.TestPlatform.Extensions.EventLogCollector.Resources.Resources;

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
/// The event log container.
/// </summary>
internal class EventLogContainer : IEventLogContainer
{
private readonly ISet<string> _eventSources;
private readonly ISet<string>? _eventSources;

private readonly ISet<EventLogEntryType> _entryTypes;
private readonly int _maxLogEntries;

private readonly DataCollectionLogger _dataCollectionLogger;

private readonly DataCollectionContext _dataCollectionContext;
private readonly DataCollectionContext? _dataCollectionContext;

/// <summary>
/// Keeps track of if we are disposed.
Expand Down Expand Up @@ -55,7 +53,7 @@ internal class EventLogContainer : IEventLogContainer
/// <param name="dataCollectionContext">
/// Data Collection Context
/// </param>
public EventLogContainer(string eventLogName, ISet<string> eventSources, ISet<EventLogEntryType> entryTypes, int maxLogEntries, DataCollectionLogger dataCollectionLogger, DataCollectionContext dataCollectionContext)
public EventLogContainer(string eventLogName, ISet<string>? eventSources, ISet<EventLogEntryType> entryTypes, int maxLogEntries, DataCollectionLogger dataCollectionLogger, DataCollectionContext? dataCollectionContext)
{
EventLog = new EventLog(eventLogName);
EventLog.EnableRaisingEvents = true;
Expand Down Expand Up @@ -119,7 +117,7 @@ public void Dispose()
/// </summary>
/// <param name="source">Source</param>
/// <param name="e">The System.Diagnostics.EntryWrittenEventArgs object describing the entry that was written.</param>
public void OnEventLogEntryWritten(object source, EntryWrittenEventArgs e)
public void OnEventLogEntryWritten(object? source, EntryWrittenEventArgs? e)
{
while (!LimitReached)
{
Expand Down
70 changes: 38 additions & 32 deletions ...taCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogDataCollector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Linq;
Expand All @@ -16,8 +17,6 @@

using Resource = Microsoft.TestPlatform.Extensions.EventLogCollector.Resources.Resources;

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
Expand Down Expand Up @@ -65,22 +64,22 @@ public class EventLogDataCollector : DataCollector
/// <summary>
/// Object containing the execution events the data collector registers for
/// </summary>
private DataCollectionEvents _events;
private DataCollectionEvents? _events;

/// <summary>
/// The sink used by the data collector to send its data
/// </summary>
private DataCollectionSink _dataSink;
private DataCollectionSink? _dataSink;

/// <summary>
/// The data collector context.
/// </summary>
private DataCollectionContext _dataCollectorContext;
private DataCollectionContext? _dataCollectorContext;

/// <summary>
/// Used by the data collector to send warnings, errors, or other messages
/// </summary>
private DataCollectionLogger _logger;
private DataCollectionLogger? _logger;

/// <summary>
/// The file helper.
Expand Down Expand Up @@ -120,18 +119,17 @@ internal EventLogDataCollector(IFileHelper fileHelper)

internal int MaxEntries { get; private set; }

internal ISet<string> EventSources { get; private set; }
internal ISet<string>? EventSources { get; private set; }

internal ISet<EventLogEntryType> EntryTypes { get; private set; }
internal ISet<EventLogEntryType>? EntryTypes { get; private set; }

internal ISet<string> EventLogNames { get; private set; }
internal ISet<string>? EventLogNames { get; private set; }

/// <summary>
/// Gets the context data.
/// </summary>
internal Dictionary<DataCollectionContext, EventLogSessionContext> ContextMap { get; }


#region DataCollector Members

/// <summary>
Expand All @@ -149,12 +147,13 @@ internal EventLogDataCollector(IFileHelper fileHelper)
/// Used by the data collector to send warnings, errors, or other messages
/// </param>
/// <param name="dataCollectionEnvironmentContext">Provides contextual information about the agent environment</param>
[MemberNotNull(nameof(_events), nameof(_dataSink), nameof(_logger))]
public override void Initialize(
XmlElement configurationElement,
XmlElement? configurationElement,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally we should accept only non-null but that would be a change of behavior so I decided to allow null.

DataCollectionEvents events!!,
DataCollectionSink dataSink!!,
DataCollectionLogger logger!!,
DataCollectionEnvironmentContext dataCollectionEnvironmentContext)
DataCollectionEnvironmentContext dataCollectionEnvironmentContext!!)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change of behavior but we weren't doing null check so if Initialize is called with null for this arg we would fail with NRE.

{
_events = events;
_dataSink = dataSink;
Expand Down Expand Up @@ -224,7 +223,7 @@ internal string WriteEventLogs(List<EventLogEntry> eventLogEntries, int maxLogEn
{
for (int i = 1; !unusedFilenameFound; i++)
{
eventLogPath = eventLogBasePath + "-" + i.ToString(CultureInfo.InvariantCulture) + ".xml";
eventLogPath = $"{eventLogBasePath}-{i.ToString(CultureInfo.InvariantCulture)}.xml";

if (!_fileHelper.Exists(eventLogPath))
{
Expand Down Expand Up @@ -267,6 +266,7 @@ internal string WriteEventLogs(List<EventLogEntry> eventLogEntries, int maxLogEn
// Write the event log file
FileTransferInformation fileTransferInformation =
new(dataCollectionContext, eventLogPath, true, _fileHelper);
TPDebug.Assert(_dataSink != null, "Initialize should have been called.");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use the initialize pattern we were talking about because this method is called from within the Initialize method.

_dataSink.SendFileAsync(fileTransferInformation);

EqtTrace.Verbose(
Expand All @@ -287,10 +287,13 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);

// Unregister events
_events.SessionStart -= _sessionStartEventHandler;
_events.SessionEnd -= _sessionEndEventHandler;
_events.TestCaseStart -= _testCaseStartEventHandler;
_events.TestCaseEnd -= _testCaseEndEventHandler;
if (_events != null)
{
_events.SessionStart -= _sessionStartEventHandler;
_events.SessionEnd -= _sessionEndEventHandler;
_events.TestCaseStart -= _testCaseStartEventHandler;
_events.TestCaseEnd -= _testCaseEndEventHandler;
}

// Unregister EventLogEntry Written.
foreach (var eventLogContainer in _eventLogContainerMap.Values)
Expand Down Expand Up @@ -352,7 +355,7 @@ private void OnTestCaseStart(object sender, TestCaseStartEventArgs e!!)
private void OnTestCaseEnd(object sender, TestCaseEndEventArgs e!!)
{
Debug.Assert(e.Context != null, "Context is null");
Debug.Assert(e.Context.HasTestCase, "Context is not for a test case");
Debug.Assert(e.Context!.HasTestCase, "Context is not for a test case");

EqtTrace.Verbose(
"EventLogDataCollector: TestCaseEnd received for test '{0}' with Test Outcome: {1}.",
Expand Down Expand Up @@ -402,13 +405,15 @@ private void WriteCollectedEventLogEntries(
kvp.Value.EventLog.EnableRaisingEvents = false;
}

// REVIEW ME: We are initializing
for (int i = context.EventLogContainerStartIndexMap[kvp.Key]; i <= context.EventLogContainerEndIndexMap[kvp.Key]; i++)
{
eventLogEntries.Add(kvp.Value.EventLogEntries[i]);
}
}
catch (Exception e)
{
TPDebug.Assert(_logger != null, "Initialize should have been called.");
_logger.LogWarning(
dataCollectionContext,
string.Format(
Expand All @@ -430,15 +435,16 @@ private void WriteCollectedEventLogEntries(
}
}

[MemberNotNull(nameof(EventLogNames))]
private void ConfigureEventLogNames(CollectorNameValueConfigurationManager collectorNameValueConfigurationManager)
{
EventLogNames = new HashSet<string>();
string eventLogs = collectorNameValueConfigurationManager[EventLogConstants.SettingEventLogs];
string? eventLogs = collectorNameValueConfigurationManager[EventLogConstants.SettingEventLogs];
if (eventLogs != null)
{
EventLogNames = ParseCommaSeparatedList(eventLogs);
EqtTrace.Verbose(
"EventLogDataCollector configuration: " + EventLogConstants.SettingEventLogs + "=" + eventLogs);
$"EventLogDataCollector configuration: {EventLogConstants.SettingEventLogs}={eventLogs}");
}
else
{
Expand All @@ -447,6 +453,8 @@ private void ConfigureEventLogNames(CollectorNameValueConfigurationManager colle
EventLogNames.Add("Application");
}

TPDebug.Assert(_logger != null && EntryTypes != null, "Initialize should have been called.");

foreach (string eventLogName in EventLogNames)
{
try
Expand All @@ -470,27 +478,27 @@ private void ConfigureEventLogNames(CollectorNameValueConfigurationManager colle
{
_logger.LogError(
_dataCollectorContext,
new EventLogCollectorException(string.Format(CultureInfo.InvariantCulture, Resource.ReadError, eventLogName, Environment.MachineName), ex));
new EventLogCollectorException(string.Format(CultureInfo.CurrentCulture, Resource.ReadError, eventLogName, Environment.MachineName), ex));
}
}
}

private void ConfigureEventSources(CollectorNameValueConfigurationManager collectorNameValueConfigurationManager)
{
string eventSourcesStr = collectorNameValueConfigurationManager[EventLogConstants.SettingEventSources];
string? eventSourcesStr = collectorNameValueConfigurationManager[EventLogConstants.SettingEventSources];
if (!string.IsNullOrEmpty(eventSourcesStr))
{
EventSources = ParseCommaSeparatedList(eventSourcesStr);
EventSources = ParseCommaSeparatedList(eventSourcesStr!);
EqtTrace.Verbose(
"EventLogDataCollector configuration: " + EventLogConstants.SettingEventSources + "="
+ EventSources);
$"EventLogDataCollector configuration: {EventLogConstants.SettingEventSources}={EventSources}");
}
}

[MemberNotNull(nameof(EntryTypes))]
private void ConfigureEntryTypes(CollectorNameValueConfigurationManager collectorNameValueConfigurationManager)
{
EntryTypes = new HashSet<EventLogEntryType>();
string entryTypesStr = collectorNameValueConfigurationManager[EventLogConstants.SettingEntryTypes];
string? entryTypesStr = collectorNameValueConfigurationManager[EventLogConstants.SettingEntryTypes];
if (entryTypesStr != null)
{
foreach (string entryTypestring in ParseCommaSeparatedList(entryTypesStr))
Expand All @@ -500,8 +508,7 @@ private void ConfigureEntryTypes(CollectorNameValueConfigurationManager collecto
}

EqtTrace.Verbose(
"EventLogDataCollector configuration: " + EventLogConstants.SettingEntryTypes + "="
+ EntryTypes);
$"EventLogDataCollector configuration: {EventLogConstants.SettingEntryTypes}={EntryTypes}");
}
else
{
Expand All @@ -513,7 +520,7 @@ private void ConfigureEntryTypes(CollectorNameValueConfigurationManager collecto

private void ConfigureMaxEntries(CollectorNameValueConfigurationManager collectorNameValueConfigurationManager)
{
string maxEntriesstring = collectorNameValueConfigurationManager[EventLogConstants.SettingMaxEntries];
string? maxEntriesstring = collectorNameValueConfigurationManager[EventLogConstants.SettingMaxEntries];
if (maxEntriesstring != null)
{
try
Expand All @@ -532,8 +539,7 @@ private void ConfigureMaxEntries(CollectorNameValueConfigurationManager collecto
}

EqtTrace.Verbose(
"EventLogDataCollector configuration: " + EventLogConstants.SettingMaxEntries + "="
+ MaxEntries);
$"EventLogDataCollector configuration: {EventLogConstants.SettingMaxEntries}={MaxEntries}");
}
else
{
Expand All @@ -553,7 +559,7 @@ private EventLogSessionContext GetEventLogSessionContext(DataCollectionContext d
if (!eventLogContainerFound)
{
string msg = string.Format(
CultureInfo.InvariantCulture,
CultureInfo.CurrentCulture,
Resource.ContextNotFoundException,
dataCollectionContext.ToString());
throw new EventLogCollectorException(msg, null);
Expand Down
7 changes: 4 additions & 3 deletions ...aCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogSessionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

#nullable disable
using System.Diagnostics.CodeAnalysis;

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

Expand Down Expand Up @@ -34,11 +33,12 @@ public EventLogSessionContext(IDictionary<string, IEventLogContainer> eventLogCo
/// <summary>
/// Gets the end index for EventLogs Entries
/// </summary>
internal Dictionary<string, int> EventLogContainerEndIndexMap { get; private set; }
internal Dictionary<string, int>? EventLogContainerEndIndexMap { get; private set; }

/// <summary>
/// Creates the end index map for EventLogs Entries
/// </summary>
[MemberNotNull(nameof(EventLogContainerEndIndexMap))]
public void CreateEventLogContainerEndIndexMap()
{
EventLogContainerEndIndexMap = new Dictionary<string, int>(_eventLogContainerMap.Count);
Expand All @@ -54,6 +54,7 @@ public void CreateEventLogContainerEndIndexMap()
/// <summary>
/// Creates the start index map for EventLogs Entries
/// </summary>
[MemberNotNull(nameof(EventLogContainerStartIndexMap))]
public void CreateEventLogContainerStartIndexMap()
{
EventLogContainerStartIndexMap = new Dictionary<string, int>(_eventLogContainerMap.Count);
Expand Down
2 changes: 0 additions & 2 deletions src/DataCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogXmlWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@

using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces;

#nullable disable

namespace Microsoft.TestPlatform.Extensions.EventLogCollector;

/// <summary>
Expand Down
Loading