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

Set discovery batch size to 1000 #3896

Merged
merged 3 commits into from
Aug 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Utilities;
public class InferRunSettingsHelper
{
private const string DesignModeNodeName = "DesignMode";
private const string BatchSizeNodeName = "BatchSize";
private const string CollectSourceInformationNodeName = "CollectSourceInformation";
private const string RunSettingsNodeName = "RunSettings";
private const string RunConfigurationNodeName = "RunConfiguration";
Expand All @@ -33,6 +34,7 @@ public class InferRunSettingsHelper
private const string TargetDevice = "TargetDevice";

private const string DesignModeNodePath = @"/RunSettings/RunConfiguration/DesignMode";
private const string BatchSizeNodePath = @"/RunSettings/RunConfiguration/BatchSize";
private const string CollectSourceInformationNodePath = @"/RunSettings/RunConfiguration/CollectSourceInformation";
private const string RunConfigurationNodePath = @"/RunSettings/RunConfiguration";
private const string TargetPlatformNodePath = @"/RunSettings/RunConfiguration/TargetPlatform";
Expand Down Expand Up @@ -204,6 +206,16 @@ public static void UpdateDesignMode(XmlDocument runSettingsDocument, bool design
AddNodeIfNotPresent(runSettingsDocument, DesignModeNodePath, DesignModeNodeName, designModeValue);
}

/// <summary>
/// Updates the <c>RunConfiguration.BatchSize</c> value for a run settings. Doesn't do anything if the value is already set.
/// </summary>
/// <param name="runSettingsDocument">Document for runsettings xml</param>
/// <param name="batchSizeValue">Value to set</param>
public static void UpdateBatchSize(XmlDocument runSettingsDocument, long batchSizeValue)
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
{
AddNodeIfNotPresent(runSettingsDocument, BatchSizeNodePath, BatchSizeNodeName, batchSizeValue);
}

/// <summary>
/// Updates the <c>RunConfiguration.CollectSourceInformation</c> value for a run settings. Doesn't do anything if the value is already set.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,5 @@ static Microsoft.VisualStudio.TestPlatform.Utilities.MSTestSettingsUtilities.Imp
static Microsoft.VisualStudio.TestPlatform.Utilities.MSTestSettingsUtilities.IsLegacyTestSettingsFile(string? settingsFile) -> bool
static Microsoft.VisualStudio.TestPlatform.Utilities.ParallelRunSettingsUtilities.UpdateRunSettingsWithParallelSettingIfNotConfigured(System.Xml.XPath.XPathNavigator! navigator) -> void
static Microsoft.VisualStudio.TestPlatform.Utilities.StringExtensions.Tokenize(this string? input, char separator, char escape) -> System.Collections.Generic.IEnumerable<string!>!
static Microsoft.VisualStudio.TestPlatform.Utilities.InferRunSettingsHelper.UpdateBatchSize(System.Xml.XmlDocument! runSettingsDocument, long batchSizeValue) -> void
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: Shouldn't the contents of this file be sorted to ensure fewer merge conflicts?

Copy link
Member Author

Choose a reason for hiding this comment

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

How does that ensure less merge conflicts?


7 changes: 6 additions & 1 deletion src/vstest.console/CommandLine/CommandLineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine;
internal class CommandLineOptions
{
/// <summary>
/// The default batch size.
/// The default batch size for Run.
/// </summary>
public const long DefaultBatchSize = 10;
Evangelink marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// The default batch size for Discovery.
/// </summary>
public const long DefaultDiscoveryBatchSize = 1000;

/// <summary>
/// The use vsix extensions key.
/// </summary>
Expand Down
31 changes: 30 additions & 1 deletion src/vstest.console/TestPlatformHelpers/TestRequestManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,16 @@ public void DiscoverTests(
runsettings,
discoveryPayload.Sources.ToList(),
discoveryEventsRegistrar,
isDiscovery: true,
out string updatedRunsettings,
out IDictionary<string, Architecture> sourceToArchitectureMap,
out IDictionary<string, Framework> sourceToFrameworkMap))
{
runsettings = updatedRunsettings;
}



Evangelink marked this conversation as resolved.
Show resolved Hide resolved
var sourceToSourceDetailMap = discoveryPayload.Sources.Select(source => new SourceDetail
{
Source = source,
Expand Down Expand Up @@ -265,6 +268,7 @@ public void RunTests(
ProtocolConfig protocolConfig)
{
EqtTrace.Info("TestRequestManager.RunTests: run tests started.");
testRunRequestPayload.RunSettings ??= "<RunSettings></RunSettings>";
var runsettings = testRunRequestPayload.RunSettings;

if (testRunRequestPayload.TestPlatformOptions != null)
Expand All @@ -281,6 +285,7 @@ public void RunTests(
runsettings!,
sources!,
testRunEventsRegistrar,
isDiscovery: false,
out string updatedRunsettings,
out IDictionary<string, Architecture> sourceToArchitectureMap,
out IDictionary<string, Framework> sourceToFrameworkMap))
Expand Down Expand Up @@ -468,7 +473,8 @@ public void StartTestSession(
if (UpdateRunSettingsIfRequired(
payload.RunSettings,
payload.Sources,
null,
registrar: null,
isDiscovery: false,
out string updatedRunsettings,
out IDictionary<string, Architecture> sourceToArchitectureMap,
out IDictionary<string, Framework> sourceToFrameworkMap))
Expand Down Expand Up @@ -652,6 +658,7 @@ private bool UpdateRunSettingsIfRequired(
string runsettingsXml,
IList<string>? sources,
IBaseTestEventsRegistrar? registrar,
bool isDiscovery,
out string updatedRunSettingsXml,
out IDictionary<string, Architecture> sourceToArchitectureMap,
out IDictionary<string, Framework> sourceToFrameworkMap)
Expand Down Expand Up @@ -800,6 +807,7 @@ private bool UpdateRunSettingsIfRequired(
settingsUpdated |= UpdateCollectSourceInformation(document, runConfiguration);
settingsUpdated |= UpdateTargetDevice(navigator, document);
settingsUpdated |= AddOrUpdateConsoleLogger(document, runConfiguration, loggerRunSettings);
settingsUpdated |= AddOrUpdateBatchSize(document, runConfiguration, isDiscovery);

updatedRunSettingsXml = navigator.OuterXml;

Expand Down Expand Up @@ -909,6 +917,27 @@ private bool UpdateDesignMode(XmlDocument document, RunConfiguration runConfigur
return updateRequired;
}

private bool AddOrUpdateBatchSize(XmlDocument document, RunConfiguration runConfiguration, bool isDiscovery)
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
{
// On run keep it as is to fall back to the current default value (which is 10 right now).
if (!isDiscovery)
{
// We did not update runnsettings.
return false;
}

Evangelink marked this conversation as resolved.
Show resolved Hide resolved

// If user is already setting batch size via runsettings or CLI args; we skip.
bool updateRequired = !runConfiguration.BatchSizeSet;
if (updateRequired)
{
InferRunSettingsHelper.UpdateBatchSize(
document,
CommandLineOptions.DefaultDiscoveryBatchSize);
}
return updateRequired;
}

private static void CheckSourcesForCompatibility(
Framework chosenFramework,
Architecture chosenPlatform,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ public void CommandLineOptionsDefaultBatchSizeIsTen()
Assert.AreEqual(10, CommandLineOptions.Instance.BatchSize);
}

[TestMethod]
public void CommandLineOptionsDiscoveryDefaultBatchSizeIsThousand()
{
Assert.AreEqual(1000, CommandLineOptions.DefaultDiscoveryBatchSize);
}

[TestMethod]
public void CommandLineOptionsDefaultTestRunStatsEventTimeoutIsOnePointFiveSec()
{
Expand Down