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.

Sign up for GitHub

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

Jump to bottom

Fix parallel discovery #3437

Merged
merged 15 commits into from
Apr 8, 2022
Merged

Fix parallel discovery #3437

merged 15 commits into from
Apr 8, 2022

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Mar 4, 2022 •
edited
Loading

Description

The previously merged #3349 PR wasn't handling parallel discovery correctly for all cases.

Related issue

Fixes AB#1493062

Replaces #3435

nohwnd
nohwnd reviewed Mar 4, 2022
View reviewed changes
...stPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelDiscoveryDataAggregatorTests.cs
@@ -21,13 +20,13 @@ public void AggregateShouldAggregateAbortedCorrectly()
{
var aggregator = new ParallelDiscoveryDataAggregator();

aggregator.Aggregate(totalTests: 5, isAborted: false);
aggregator.Aggregate(new(totalTests: 5, isAborted: false));
Copy link
Member

Choose a reason for hiding this comment

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

I might be too tired today, but what is the change in syntax? is that going from method call with parameters to, method call that takes a single object, which has non-default constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

The aggregate used to take 2 args (total tests and is aborted), now it takes the event arg (that has an overload with the previous 2 args).

@Evangelink Evangelink force-pushed the fix-parallel-discovery-2 branch 2 times, most recently from a8561c8 to 0d22ead Compare March 7, 2022 12:53
@Evangelink Evangelink changed the title Fix parallel discovery 2 Fix parallel discovery Mar 7, 2022
@Evangelink Evangelink marked this pull request as ready for review March 7, 2022 12:53
@Evangelink
Copy link
Member Author

I still need to make some manual tests before we merge this PR.

@Evangelink Evangelink marked this pull request as draft March 8, 2022 08:09
@Evangelink Evangelink marked this pull request as ready for review March 8, 2022 18:16
@Evangelink Evangelink requested a review from nohwnd March 9, 2022 15:13
@Evangelink Evangelink enabled auto-merge (squash) March 15, 2022 14:51
@Evangelink Evangelink force-pushed the fix-parallel-discovery-2 branch 2 times, most recently from f2f9030 to c77fed8 Compare March 21, 2022 15:44
Evangelink
Evangelink commented Mar 24, 2022
View reviewed changes
test/Microsoft.TestPlatform.AcceptanceTests/DotnetTestTests.cs
@@ -1,9 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to this file are unrelated to this PR but fixes the failing acceptance tests that were blocking this PR.

Evangelink
Evangelink commented Mar 24, 2022
View reviewed changes
test/Microsoft.TestPlatform.AcceptanceTests/AcceptanceTestBase.cs
@@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.IO;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to this file are unrelated to this PR but fixes the failing acceptance tests that were blocking this PR.

Evangelink
Evangelink commented Mar 24, 2022
View reviewed changes
test/Microsoft.TestPlatform.AcceptanceTests/ResultsDirectoryTests.cs
@@ -1,13 +1,12 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to this file are unrelated to this PR but fixes the failing acceptance tests that were blocking this PR.

Evangelink
Evangelink commented Mar 24, 2022
View reviewed changes
test/Microsoft.TestPlatform.AcceptanceTests/RunsettingsTests.cs
@@ -8,8 +8,6 @@
using Microsoft.TestPlatform.TestUtilities;
using Microsoft.VisualStudio.TestTools.UnitTesting;

#nullable disable
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to this file are unrelated to this PR but fixes the failing acceptance tests that were blocking this PR.

@Evangelink Evangelink requested a review from nohwnd March 24, 2022 15:18
Evangelink
Evangelink commented Apr 1, 2022
View reviewed changes
src/Microsoft.TestPlatform.Common/Microsoft.TestPlatform.Common.csproj Outdated
@@ -6,11 +6,12 @@
<Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
<PropertyGroup>
<AssemblyName>Microsoft.VisualStudio.TestPlatform.Common</AssemblyName>
<TargetFrameworks>netstandard2.0;netstandard1.3;net451</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netstandard1.3;net451;net6.0</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding net6.0 because I need a TFM greater or equal to netcoreapp3.0 so that we have a dll where NullableAttributes.cs is not included. Otherwise we get an error that says the attribute comes from 2 locations (system and this). I am using net6.0 because we are already targeting this for DotNetBuildFromSource.

Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm. This is not being shipped in any of our Nugets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have just double checked and running build.cmd locally doesn't produce the net6.0 of the dll in the artifacts folder.

Evangelink
Evangelink commented Apr 1, 2022
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/Debug.cs

namespace Microsoft.TestPlatform;

[SuppressMessage("ApiDesign", "RS0030:Do not used banned APIs", Justification = "Replacement API to allow nullable hints for compiler")]
Copy link
Member Author

Choose a reason for hiding this comment

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

I could remove this because I haven't yet banned the other symbol but keeping it would simplify merge for later changes.

Copy link
Member

Choose a reason for hiding this comment

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

Keep it.

Evangelink
Evangelink commented Apr 1, 2022
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/Debug.cs
namespace Microsoft.TestPlatform;

[SuppressMessage("ApiDesign", "RS0030:Do not used banned APIs", Justification = "Replacement API to allow nullable hints for compiler")]
internal static class Debug
Copy link
Member Author

Choose a reason for hiding this comment

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

I am always forcing this one but FYI in recent versions the system Debug.Assert methods already support it.

Evangelink
Evangelink commented Apr 1, 2022
View reviewed changes
src/Microsoft.TestPlatform.ObjectModel/Client/Events/DiscoveryCompleteEventArgs.cs
// This event is always raised from the client side, while the total count of tests is maintained
// only at the testhost end. In case of a discovery abort (various reasons including crash), it is
// not possible to get a list of total tests from testhost. Hence we enforce a -1 count.
Debug.Assert((!isAborted || -1 == totalTests), "If discovery request is aborted totalTest should be -1.");
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to remove this comment because we are now also using this event args inside the parallel discovery.

Evangelink
Evangelink commented Apr 1, 2022
View reviewed changes
src/Microsoft.TestPlatform.ObjectModel/Client/Events/DiscoveryCompleteEventArgs.cs
/// <param name="fullyDiscoveredSources">List of fully discovered sources</param>
/// <param name="partiallyDiscoveredSources">List of partially discovered sources</param>
/// <param name="notDiscoveredSources">List of not discovered sources</param>
public DiscoveryCompleteEventArgs(
Copy link
Member Author

Choose a reason for hiding this comment

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

@cvpoienaru added a new ctor with this signature + the extensions dictionary but this public ctor is not yet released so we can just go with only one overload.

nohwnd
nohwnd reviewed Apr 1, 2022
View reviewed changes
playground/TestPlatform.Playground/Program.cs Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/Microsoft.TestPlatform.Common.csproj Outdated
@@ -6,11 +6,12 @@
<Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
<PropertyGroup>
<AssemblyName>Microsoft.VisualStudio.TestPlatform.Common</AssemblyName>
<TargetFrameworks>netstandard2.0;netstandard1.3;net451</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netstandard1.3;net451;net6.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm. This is not being shipped in any of our Nugets?

src/Microsoft.TestPlatform.Common/Utilities/Debug.cs

namespace Microsoft.TestPlatform;

[SuppressMessage("ApiDesign", "RS0030:Do not used banned APIs", Justification = "Replacement API to allow nullable hints for compiler")]
Copy link
Member

Choose a reason for hiding this comment

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

Keep it.

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelDiscoveryDataAggregator.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelDiscoveryDataAggregator.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelDiscoveryDataAggregator.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs Outdated Show resolved Hide resolved
@Evangelink
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Evangelink
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Evangelink and others added 15 commits April 8, 2022 10:06
@Evangelink
First test with logic inside ParalleDiscoverEventsHandler
2b75583
@Evangelink
Fix parallel discovery
674c3ed
@Evangelink
Update code and add tests
31f6307
@Evangelink
Handle more cases
9dc3cfe
@Evangelink
Tests tests are fully discovered when complete event is raised and th…
44ae540 
…ere was no abort
@Evangelink
Cleanup
3ed330d
@Evangelink
Try to fix unrelated failing acceptance tests
294b1c0
@Evangelink
Fix merge
f9106b8
@Evangelink
Share data aggregator with proxy so that state is not synced only on …
4f74609 
…completion
@Evangelink
Refactor and fix rebase conflicts
0bf9864
@Evangelink
Refactoring
e0acba1
@Evangelink
Make playground test discovery handler more useful for debugging
fd5e6a4
@Evangelink @nohwnd
Update src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/Par…
670c1c0 
…allelDiscoveryDataAggregator.cs

Co-authored-by: Jakub Jareš <[email protected]>
@Evangelink
Update comment
36eccdc
@Evangelink
Cleanup and tidy up code
63b1c1e
@Evangelink Evangelink enabled auto-merge (squash) April 8, 2022 08:14
@Evangelink Evangelink merged commit de3e066 into microsoft:main Apr 8, 2022
@Evangelink Evangelink deleted the fix-parallel-discovery-2 branch April 8, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nohwnd nohwnd nohwnd approved these changes

@MarcoRossignoli MarcoRossignoli Awaiting requested review from MarcoRossignoli MarcoRossignoli is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Evangelink @nohwnd