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

Added support for ManagedType and ManagedClass #737

Merged
merged 22 commits into from
Nov 24, 2020
Merged

Conversation

Haplois
Copy link
Contributor

@Haplois Haplois commented Oct 22, 2020

microsoft/vstest#2611 needs to be merged, and published into the package repository before merging this PR.

@microsoft microsoft deleted a comment from azure-pipelines bot Oct 22, 2020
@microsoft microsoft deleted a comment from azure-pipelines bot Oct 22, 2020
@microsoft microsoft deleted a comment from azure-pipelines bot Oct 22, 2020
@Haplois Haplois changed the title Updating TestPlarform version FQN work Nov 2, 2020
Added support for `ManagedType` and `ManagedMethod` into various classes. Also centralized TestPlarform version, and some refactoring.
@microsoft microsoft deleted a comment from azure-pipelines bot Nov 3, 2020
@microsoft microsoft deleted a comment from azure-pipelines bot Nov 3, 2020
@peterwald
Copy link
Member

LGTM with a few minor comments. It's encouraging to see that the addition of the two properties seems to be relatively simple. Of course, we still need to add support for parameter/argument sets.

@Haplois Haplois self-assigned this Nov 24, 2020
Copy link
Member

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

Looks mostly good, I would review if #if NETCOREAPP1_1 || NETCOREAPP2_1 can be replaced witch check for just netcoreapp to help us in the future if we need to multitarget this further.

function Sync-PackageVersions {
$versionsFile = "$PSScriptRoot\build\TestFx.Versions.targets"

$versionsRegex = '(?mi)<(TestPlatformVersion.*?)>(.*?)<\/TestPlatformVersion>'
Copy link
Member

@nohwnd nohwnd Nov 24, 2020

Choose a reason for hiding this comment

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

nit: Not sure what the ?mi option would do here. It is definitely case insensitive by default ( -split and -isplit is case insensitive, -csplit is case sensitive). And you are passing the whole file as one string (you used -Raw). If you have a strong reason I would like to know to learn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some (like test/E2ETests/Automation.CLI/packages.config) files we reference more than one package, this is to catch all references and update those.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that is the default behavior of -replace:

PS > Get-Content ~/Desktop/abc.txt

abc
abc abcabc
abc

PS > (Get-Content ~/Desktop/abc.txt -Raw) -replace "abc", "ggg"

ggg       
ggg gggggg
ggg    

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'll have another PR after this and move to use PackageReference instead of "NuGet.config" files. With that PR, I'll remove this logic too.

scripts/Build.ps1 Outdated Show resolved Hide resolved
scripts/common.lib.ps1 Show resolved Hide resolved
@Haplois Haplois merged commit 76f0f89 into microsoft:master Nov 24, 2020
@Haplois Haplois deleted the ns10 branch November 24, 2020 09:18
@Haplois Haplois changed the title FQN work Added support for ManagedType and ManagedClass Nov 26, 2020

if (this.TestMethod.HasManagedMethodAndType)
{
testCase = new TestCase(fullName, this.TestMethod.ManagedType, this.TestMethod.ManagedMethod, TestAdapter.Constants.ExecutorUri, this.TestMethod.AssemblyName);
Copy link

@shyamnamboodiripad shyamnamboodiripad Jan 8, 2021

Choose a reason for hiding this comment

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

@Haplois Won't this fail If the version of mstest adapter that includes the current FQN changes is loaded in VS 2017 where the TP OM that will be loaded will not contain this TestCase constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will fail. I am moving these changes to a separate assembly and NuGet package. I'll have a PR soon.

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.

4 participants