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

Enable parallelization of acceptance tests #3268

Merged
merged 12 commits into from
Jan 21, 2022

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Jan 20, 2022

Description

Update acceptance tests to enable parallelization.

Related issue

Fixes AB#1463179

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.

Nice. I think we jumped too ahead of ourselves with the WrapperContext, the additional property is used in only 1 or two tests, and we initialize it in the constructor, so I would prefer having the tests use the IVSTestConsoleWrapper directly, to minimize the changes. And where it is needed you can add additional property to the test class that would hold the additional info. Maybe GetVstestConsoleWrapper could return out parameter that would have an object holding that info?

this.ValidateSummaryStatus(2, 2, 2);

string mergedFile = Directory.GetFiles(resultsDir, "MergedFile.txt", SearchOption.AllDirectories).Single();
string mergedFile = Directory.GetFiles(workspace.Path, "MergedFile.txt", SearchOption.AllDirectories).Single();
Copy link
Contributor

Choose a reason for hiding this comment

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

we can also think to add GetFiles to workspace...it's a common task search for files.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I had that in my original proposal as well. So maybe as a second step?

@@ -19,3 +20,6 @@

// The following GUID is for the ID of the typelib if this project is exposed to COM
[assembly: Guid("755996fa-672a-4272-9776-7f707a520058")]

// Enable IAP at method level with as many threads as possible based on CPU and core count.
[assembly: Parallelize(Workers = 0, Scope = ExecutionScope.MethodLevel)]
Copy link
Contributor

Choose a reason for hiding this comment

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

💘

{
// this creates dotnet hosted vstest console and 2 testhosts one of which is hosted
// in dotnet, so we have two dotnet + 1 testhost.exe
expectedProcessCreated = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it ok to remove?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this was a special case for the thing described in the comment. We no longer count processes by observing which process started during run. But rather by counting the number of diagnostic logs for host that were produced during run. This makes it possible to run in parallel, and is less fragile in case where vstest.console and testhost both run under dotnet.exe apphost (which is the special case above where we have 2 dlls, running in 2 dotnet testhost.dll, but also 1 dotnet vstest.console.dll, and we could not distinguish which dotnet.exe is actually hosting testhost, and which is hosting vstest.console, so we counted 3 in total).

@@ -36,7 +37,7 @@ public static void ClassCleanup()
{
try
{
Directory.Delete(privateX64Installation, true);
new DirectoryInfo(privateX64Installation).Parent.Delete(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why Parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the init we do a temp dir with a subfolder named x64 and we keep a reference to the sub-folder, so I want to clean the right folder (i.e. the parent).

@nohwnd nohwnd enabled auto-merge (squash) January 21, 2022 15:22
@nohwnd nohwnd merged commit 112392b into microsoft:main Jan 21, 2022
@Evangelink Evangelink deleted the acceptance-tests-parallel branch January 21, 2022 15:23
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.

3 participants