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 dictionary key collision of test run parameters. #308

Closed
wants to merge 2 commits into from
Closed

Fix dictionary key collision of test run parameters. #308

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03d6a6d
Fix dictionary key collision of test run parameters.
CodeCyclone Nov 3, 2017
fb262b5
Merge branch 'master' into chbeck/AddRunSettingsKeepEngineAlive
CodeCyclone Nov 8, 2017
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
32 changes: 8 additions & 24 deletions src/Adapter/MSTest.CoreAdapter/Execution/TestExecutionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,9 @@ public class TestExecutionManager
/// </summary>
private TestRunCancellationToken cancellationToken;

/// <summary>
/// Dictionary for test run parameters
/// </summary>
private IDictionary<string, object> sessionParameters;

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1810:InitializeReferenceTypeStaticFieldsInline", Justification = "Need to over-write the keys in dictionary.")]
public TestExecutionManager()
{
this.TestMethodFilter = new TestMethodFilter();
this.sessionParameters = new Dictionary<string, object>();
}

/// <summary>
Expand Down Expand Up @@ -67,9 +60,6 @@ public void RunTests(IEnumerable<TestCase> tests, IRunContext runContext, IFrame

var isDeploymentDone = PlatformServiceProvider.Instance.TestDeployment.Deploy(tests, runContext, frameworkHandle);

// Placing this after deployment since we need information post deployment that we pass in as properties.
this.CacheSessionParameters(runContext, frameworkHandle);
Copy link
Member

Choose a reason for hiding this comment

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

We would want to call CacheSessionParameters() here itself because we want to process the testRunParameters only once for a test run. Placing this call inside ExecuteTestsInSource() would re-evaluate the testRunParameters for each source and hence, regressing the performance.

Copy link
Author

Choose a reason for hiding this comment

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

The problem we have is because the test execution engine is left running which keeps the TestExecutionManager instance alive, there are key collisions when populating in CacheSessionParameters(). Alternatively we could change CacheSessionParameters() to use this.sessionParameters[kvp.Key] = kvp.Value which then just overwrites the parameters in the event of collision. The downside to this is you might have bleed-over of settings from a previous run which we saw when we renamed a test run parameter and still saw the old name in the TestContext.Properties collection along with the new name. Tests could behave indeterminately which is an trait you don't want in test frameworks.
We decided to load it every run because RunSettingsUtilities.GetTestRunParameters is pulling from an in-memory cache of the runsettings so it should execute very quickly and its the only sure fire way to prevent the bleed-over effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below. You could choose to rename this API to SetSessionParameters if you choose to so it is explicit we reset this data here.


// Execute the tests
this.ExecuteTests(tests, runContext, frameworkHandle, isDeploymentDone);

Expand Down Expand Up @@ -107,9 +97,6 @@ public void RunTests(IEnumerable<string> sources, IRunContext runContext, IFrame

bool isDeploymentDone = PlatformServiceProvider.Instance.TestDeployment.Deploy(tests, runContext, frameworkHandle);

// Placing this after deployment since we need information post deployment that we pass in as properties.
this.CacheSessionParameters(runContext, frameworkHandle);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.


// Run tests.
this.ExecuteTests(tests, runContext, frameworkHandle, isDeploymentDone);

Expand Down Expand Up @@ -236,6 +223,8 @@ private void ExecuteTestsWithTestRunner(

if (!filterHasError)
{
var sessionParameters = this.GetSessionParameters(runContext, testExecutionRecorder);
Copy link
Member

Choose a reason for hiding this comment

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

Call to CacheSessionParameters() should not be placed here. Same as above.


foreach (var currentTest in tests)
{
// Skip test if not fitting filter criteria.
Expand Down Expand Up @@ -267,9 +256,9 @@ private void ExecuteTestsWithTestRunner(
// and are merged with session level parameters
var sourceLevelParameters = PlatformServiceProvider.Instance.SettingsProvider.GetProperties(source);

if (this.sessionParameters != null && this.sessionParameters.Count > 0)
if (sessionParameters != null && sessionParameters.Count > 0)
{
sourceLevelParameters = sourceLevelParameters.Concat(this.sessionParameters).ToDictionary(x => x.Key, x => x.Value);
sourceLevelParameters = sourceLevelParameters.Concat(sessionParameters).ToDictionary(x => x.Key, x => x.Value);
}

unitTestResult = testRunner.RunSingleTest(unitTestElement.TestMethod, sourceLevelParameters);
Expand Down Expand Up @@ -313,26 +302,21 @@ private void ExecuteTestsWithTestRunner(
}

[SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Requirement is to handle errors in user specified run parameters")]
private void CacheSessionParameters(IRunContext runContext, ITestExecutionRecorder testExecutionRecorder)
private IDictionary<string, object> GetSessionParameters(IRunContext runContext, ITestExecutionRecorder testExecutionRecorder)
{
if (!string.IsNullOrEmpty(runContext?.RunSettings?.SettingsXml))
{
try
{
var testRunParameters = RunSettingsUtilities.GetTestRunParameters(runContext.RunSettings.SettingsXml);
if (testRunParameters != null)
Copy link
Member

Choose a reason for hiding this comment

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

To address the problem of key value already being present in the dictionary, we can clear the sessionParameters object here and keep rest of the flow as it.

Copy link
Author

Choose a reason for hiding this comment

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

Clearing sessionParameters would happen every time in the event of test run parameters are used, the performance would be the same with the change I propose. The only optimization would be if test run parameters are not used and the collision can't occur.
Alternatively I could do this.sessionParameters[kvp.Key] = kvp.Value instead and rename the function back to CacheSessionParameters(), keeping the original flow. But as I mentioned above you will have bleed-over of properties between runs which could have indeterminately affect the run depending on the order of execution and how the tests are designed. But the performance shouldn't be impacted at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CodeCyclone : Across sessions it should be ok to re-calculate TestRunParameters. That could potentially even change when the runsettings is changed. But for one session we should just calculate session parameters once as opposed to doing so for every source. For 100 test assemblies in a session we would be recalculating the same data 100 times.
this.sessionParameters[kvp.Key] = kvp.Value as you stated wouldn't be the right thing to do here - the run carries forward data from the previous session that could lead to all sorts of problems.
It would be great if you could clear the data from the previous session and reset it in this API as @jayaranigarg suggested.

{
foreach (var kvp in testRunParameters)
{
this.sessionParameters.Add(kvp);
}
}
return RunSettingsUtilities.GetTestRunParameters(runContext.RunSettings.SettingsXml);
}
catch (Exception ex)
{
testExecutionRecorder.SendMessage(TestMessageLevel.Error, ex.Message);
}
}

return new Dictionary<string, object>();
}

/// <summary>
Expand Down