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

Adapter Failed to Load #958 #2156

Merged
merged 25 commits into from Sep 25, 2019
Merged

Conversation

DineshChirnanchu
Copy link
Contributor

No errors reported for adapters that are not loaded because of 'copied from internet' bit on adapter binary

#958

@DineshChirnanchu
Copy link
Contributor Author

@mayankbansal018 @singhsarab

Pushed fixes for failing tests, kindly verify and share your feedback

var pathToAdditionalExtensions = this.dataSerializer.DeserializePayload<IEnumerable<string>>(message);
jobQueue.QueueJob(
() =>
testHostManagerFactory.GetDiscoveryManager().Initialize(pathToAdditionalExtensions), 0);
testHostManagerFactory.GetDiscoveryManager().Initialize(pathToAdditionalExtensions, discoveryEventsHandler), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

discoveryEventsHandler [](start = 116, length = 22)

should be testExtensionInitializeEventsHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will go ahead with this once we finalize the permanent fix for it

@DineshChirnanchu
Copy link
Contributor Author

@singhsarab , @mayankbansal018

Overriding the TestMessageLevel From Error to Warning in ExecutionManager.cs and DiscoveryManager.cs files to get through the build checks.
Added resource string for error message
And the changes above are working well in local environment

@DineshChirnanchu
Copy link
Contributor Author

Pushed changes @singhsarab @mayankbansal018
-Added new tests
-Updated Error message

namespace Microsoft.VisualStudio.TestPlatform.Common
{
using System.Collections.Generic;
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

has been removed

@@ -316,12 +316,12 @@ private static void LogWarningOnNoTestsDiscovered(IEnumerable<string> sources, s
}
else
{
//Should be logged only when tests found - if adapter failed to load then there would be no tests to process
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

this.sessionMessageLogger.TestRunMessage += this.TestSessionMessageHandler;
}

private void TestSessionMessageHandler(object sender, TestRunMessageEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

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

move private to bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved under private methods region

@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.



namespace Microsoft.VisualStudio.TestPlatform.Common
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this file changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

@@ -247,6 +249,11 @@ private void TestSessionMessageHandler(object sender, TestRunMessageEventArgs e)

if (this.testDiscoveryEventsHandler != null)
{
if (e.Level == TestMessageLevel.Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error [](start = 48, length = 5)

Do not downgrade error to warnings, this handler is also invoked when Adapters(Extensions) send something back as error, we cannot reduce their messages as warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Level downgrade lines, and now it follows whatever the Level it gets.

@@ -88,6 +93,8 @@ public void Initialize(IEnumerable<string> pathToAdditionalExtensions)
ITestCaseEventsHandler testCaseEventsHandler,
ITestRunEventsHandler runEventsHandler)
{
//unsubscrive session logger
this.sessionMessageLogger.TestRunMessage -= this.TestSessionMessageHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done in Initialize itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in to the Initialize()

{
if (this.testMessageEventsHandler != null)
{
if (e.Level == TestMessageLevel.Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as discovery handler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Level downgrade lines, and now it follows whatever the Level it gets.

() => { });

//Act
this.discoveryManager.Initialize(new List<string> { assemblyLocation }, mockLogger.Object);
Copy link
Contributor

Choose a reason for hiding this comment

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

assemblyLocation [](start = 64, length = 16)

make the message as appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary test and updated message.

@DineshChirnanchu
Copy link
Contributor Author

@mayankbansal018 , suggested changes pushed, kindly have a look

Copy link
Contributor

@mayankbansal018 mayankbansal018 left a comment

Choose a reason for hiding this comment

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

:shipit:

@DineshChirnanchu DineshChirnanchu changed the title Adapter Failed to Load #958 Adapter Failed to Load Sep 25, 2019
@DineshChirnanchu DineshChirnanchu changed the title Adapter Failed to Load Adapter Failed to Load #958 Sep 25, 2019
@DineshChirnanchu
Copy link
Contributor Author

@mayankbansal018

Reverted unnecessary changes.
Verified 'No tests available' message behavior in old one and mine seems it is same, so issue with that.
Left the ExecutionManager unit test commented for future reference.

@mayankbansal018 mayankbansal018 merged commit 33a2be6 into microsoft:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants