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

Adding Microsoft.VisualStudio.ArchitectureTools.PEReader.dll to Extensions #2041

Merged
merged 4 commits into from Jun 6, 2019

Conversation

vagisha-nidhi
Copy link
Contributor

@vagisha-nidhi vagisha-nidhi commented Jun 3, 2019

Description

AppDomain.CurrentDomain.GetAssemblies() tries to load Microsoft.VisualStudio.TraceDataCollector from Extensions in VS 16.1 due to this change: https://github.com/microsoft/vstest/pull/1991/files
traceDataCollectorAssembly.GetTypes() fails because it is not able to find Microsoft.VisualStudio.ArchitectureTools.PEReader.dll in Extensions folder.

Validations:

  1. Running tests in x64 and x86 architecture.
  2. Running tests with code coverage on VS and without code coverage.

Related issue

#2008

Copy link
Contributor

@singhsarab singhsarab left a comment

Choose a reason for hiding this comment

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

Should we add a test for this?

@vagisha-nidhi
Copy link
Contributor Author

Should we add a test for this?

What kind of test? Kind of E2E test that loads up datacollectors?

@singhsarab
Copy link
Contributor

Should we add a test for this?

What kind of test? Kind of E2E test that loads up datacollectors?

Yes. E2E should do. I am actually surprised that this dint get caught by any of existing tests.

@cltshivash
Copy link
Contributor

@vagisha-nidhi Please update with the set of validations you had done as well. (code coverage and non-code coverage).

@vagisha-nidhi
Copy link
Contributor Author

@vagisha-nidhi Please update with the set of validations you had done as well. (code coverage and non-code coverage).

Updated in the description.

@vagisha-nidhi
Copy link
Contributor Author

Should we add a test for this?

What kind of test? Kind of E2E test that loads up datacollectors?

Yes. E2E should do. I am actually surprised that this dint get caught by any of existing tests.

@singhsarab @cltshivash Added E2E test

{
AcceptanceTestBase.SetTestEnvironment(this.testEnvironment, runnerInfo);

var arguments = PrepareArguments(GetAssetFullPath("TraceCollectorLoader.dll", "netcoreapp2.0"), string.Empty, string.Empty, this.FrameworkArgValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding only netcoreapp2.0 since AppDomain is only available after netcoreapp2.0.
Documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this run for net451 then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

And where is the datacollector getting enabled, I need to understand this better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We load the datacollectors only in the case of netcore as this change was made only in DotnetTestHostManager here. So net451 run without error.
var allAssemblies = AppDomain.CurrentDomain.GetAssemblies(); in the test loads datacollectors including the trace datacollector.
When we attempt to get the types of the trace datacollector, in var typeInfo = assembly.GetTypes(); it breaks with the error.

using System;
using System.Linq;

namespace TraceCollectorLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the project is slightly misleading. Rename it to what is actually doing, something related to GetAssemblies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@cltshivash cltshivash left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Contributor

@cltshivash cltshivash left a comment

Choose a reason for hiding this comment

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

:shipit:

@vagisha-nidhi vagisha-nidhi merged commit 537412c into microsoft:master Jun 6, 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