Fix: Logger attachments not coming in vsts test run #1431
Conversation
abhishkk
requested review from
nigurr,
cltshivash,
singhsarab and
mayankbansal018
| var message = this.dataSerializer.DeserializeMessage(rawMessage); | ||
| var discoveryCompletePayload = this.LoggerManager.LoggersInitialized || this.requestData.IsTelemetryOptedIn ? | ||
| this.dataSerializer.DeserializePayload<DiscoveryCompletePayload>(message) : default(DiscoveryCompletePayload); | ||
| rawMessage = UpdateRawMessageWithTelemetryInfo(discoveryCompletePayload, message) ?? rawMessage; |
There was a problem hiding this comment.
UpdateRawMessageWithTelemetryInfo [](start = 29, length = 33)
what if telemetry is disabled, do we still want to add telemetry data to message? #Closed
There was a problem hiding this comment.
No we are not adding telemetry data when its disabled #Closed
| { | ||
| var message = this.dataSerializer.DeserializeMessage(rawMessage); | ||
| var discoveryCompletePayload = this.LoggerManager.LoggersInitialized || this.requestData.IsTelemetryOptedIn ? |
There was a problem hiding this comment.
this.LoggerManager.LoggersInitialized || this.requestData.IsTel [](start = 47, length = 63)
are these checks needed again? You have already checked above that either Loggers or telemetry is enabled #Closed
| } | ||
| if (string.Equals(message?.MessageType, MessageType.ExecutionComplete)) | ||
| { | ||
| var testRunCompletePayload = this.LoggerManager.LoggersInitialized || this.requestData.IsTelemetryOptedIn ? |
There was a problem hiding this comment.
same comment as in DiscoveryRequest #Closed
| if (!designMode) | ||
| { | ||
| AddOrUpdateConsoleLogger(document, runsettingsXml); | ||
| settingsUpdated = true; | ||
| } | ||
| else | ||
| { | ||
| settingsUpdated = settingsUpdated || UpdateConsoleLoggerIfExists(document, runsettingsXml); |
There was a problem hiding this comment.
Why do we want to enable Console logger in DesignMode?
There was a problem hiding this comment.
If user has added in runsettings, we are allowing console logger. #Closed
| this.LoggerManager.HandleDiscoveryComplete(discoveryCompleteEventArgs); | ||
| this.LoggerManager.Dispose(); |
There was a problem hiding this comment.
Where are we disposing now ?
There was a problem hiding this comment.
In LoggerManager.HandleDiscoveryCompelte
| /// <param name="runsettingsXml">Runsettings xml.</param> | ||
| /// <returns>True if updated console logger in runsettings successfully.</returns> | ||
| private bool UpdateConsoleLoggerIfExists(XmlDocument document, string runsettingsXml) | ||
| { | ||
| var dummyConsoleLogger = new LoggerSettings |
There was a problem hiding this comment.
dummyConsoleLogger [](start = 16, length = 18)
nit: default*
| @@ -31,7 +31,7 @@ public TestRunCompleteEventArgs(ITestRunStatistics stats, bool isCanceled, bool | |||
| this.IsCanceled = isCanceled; | |||
| this.IsAborted = isAborted; | |||
| this.Error = error; | |||
| this.AttachmentSets = attachmentSets; | |||
| this.AttachmentSets = attachmentSets ?? new Collection<AttachmentSet>(); // Ensuring attachmentSets are not null, so that new attachmentSets can be combined whenever required. | |||
There was a problem hiding this comment.
Please put a comment why we are now initialization it
| discoveryRequest.HandleDiscoveredTests(null); | ||
|
|
||
| loggerManager.Verify(lm => lm.HandleDiscoveredTests(It.IsAny<DiscoveredTestsEventArgs>()), Times.Once); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Need test to validate that HandleDiscoveryComplete of Logger Manager is invoked only once,if the message comes from both HandleRawMessage, & HandleDiscoveryComplete
There was a problem hiding this comment.
UT are there for this scenario.
| testRunRequest.HandleLogMessage(TestMessageLevel.Error, "hello"); | ||
| loggerManager.Verify(lm => lm.HandleTestRunMessage(It.IsAny<TestRunMessageEventArgs>()), Times.Once); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Same as in discovery tests
There was a problem hiding this comment.
UTs are present for this scenario.
Description
In vsts, HandleRawMessage was sending complete event before HandleTestRunComplete can call loggers. Thus attachments created by loggers were not coming
Fix
Similar to data collector, we have moved loggers invokation to HandleRawMessage.