Fix dictionary key collision of test run parameters. #308
Conversation
When 'Keep Test Execution Engine Running' in VS, the TestExecutionManager is reused and tests that contain TestRunSettings will encounter key collision when being added between runs and bleedover of properties will occur.
|
This is the type of errors this change fixes: That error can cause VSTS to fail the test step. I didn't create a new test as it required a larger change to the TestExecutionManagerTests for capturing the error text and instead validated it manually. |
|
|
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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); | |||
| @@ -236,6 +223,8 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext runCo | |||
|
|
|||
| if (!filterHasError) | |||
| { | |||
| var sessionParameters = this.GetSessionParameters(runContext, testExecutionRecorder); | |||
There was a problem hiding this comment.
Call to CacheSessionParameters() should not be placed here. Same as above.
| { | ||
| if (!string.IsNullOrEmpty(runContext?.RunSettings?.SettingsXml)) | ||
| { | ||
| try | ||
| { | ||
| var testRunParameters = RunSettingsUtilities.GetTestRunParameters(runContext.RunSettings.SettingsXml); | ||
| if (testRunParameters != null) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
@CodeCyclone : Thank you for your contribution but I believe there is some confusion in the concept of Caching testsession parameters. |
|
Fixed this issue in another PR. Punting this PR. |
* Fix for issue where xlftool.exe is not able to update neutral xlf file if we update any existing resource string * nitpick * Localize vsix * Publish sattelite assembly
When 'Keep Test Execution Engine Running' in VS, the TestExecutionManager is reused and tests that contain TestRunSettings will encounter key collision when being added between runs and bleedover of properties will occur.