Mitigate intermittent failures in access denied tests. #4788
Conversation
| @{cmdline = "Remove-Item -Path $protectedPath"; expectedError = "RemoveItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.RemoveItemCommand"; testId = 'RemoveItem'} | ||
| ) { | ||
| param ($cmdline, $expectedError) | ||
| param ($cmdline, $expectedError, $testId) |
There was a problem hiding this comment.
Rather than adding a TestId param, you can either just use cmdline or use $cmdline.split(' ')[0] to get the same as what you have. Using the cmdline may be better if new test cases for the same command get added in the future.
There was a problem hiding this comment.
Also, line 238 to check existence of $errfile should be removed since it'll always exist since you're creating it before the test.
There was a problem hiding this comment.
I considered using split but it won't solve the problem; splitting on space gives the cmdlet name. I also considered using the expected error id but that could end up colliding as well. The test id was the most straight forward solution. I'll add a comment to the test cases to call the unique requirement out specifically.
As to the errFile test, I'll remove it.
There was a problem hiding this comment.
I think it would be simpler to just use the entirety of $cmdline in place of $testid (just put it in quotes on line 229).
There was a problem hiding this comment.
@SteveL-MSFT that's not reliable; there could be characters in the command-line that are not valid for a file name. For example, the '|' is not a valid filename character.
There was a problem hiding this comment.
This is only the contents of the file and not the filename we're talking about
There was a problem hiding this comment.
The testid parameter is used to generate the test-specific error and done file names. This is being done to address the issue of the launched PowerShell process taking longer than the timeout to shutdown. As observed in the nightly build, this can cause a race between the current test and next test if the PowerShell process doesn't exit before the next test starts.
I've changed the parameter name to fileNameBase for clarity.
|
@dantraMSFT I think you misunderstood my feedback. You don't need a different errfile for each variation of the test. The important part is having the context in the contents so when it fails, the engineer knows which variation it failed on and why. My suggestion is to get rid of $fileBaseName, it's unncessary. Just put the contents of $cmdline into $errFile. Since the validation checks the contents of $errFile and expects $expectedError, it won't match and you'll see the context message in $errFile |
|
I'm closing this to use a different approach to handling the timeout issue. |
…ilenames (void having to declare a parameter explicitly)
|
Discussed offline with Steve; will leave the test-specific error filename but generate it from a hash of $cmdline instead of requiring it's definition in the test case data. |
|
@dantraMSFT Does the PR description needs to be updated based on your offline discussion with Steve? |
|
@daxian-dbw: no, the gist was to avoid the extra parameter. We consider attempting to use start-process but that proved a dead end. |
|
@dantraMSFT thanks for the clarification. |
4 participants
Fix #4561
This change mitigates intermittent timeout failures in the access denied tests as well as fixes unintended cross-test dependencies. If PowerShell takes longer that 10 seconds to complete a race can cause the subsequent test to pick up the output file of the previous test and fail (i.e., the expected output error file already exists from the previous test because it was created by the long running PowerShell process after the second test started.) Worse case, the failure cascades causing the race to propogate through the remaining tests in the set.
The fix is two fold:
1: Increase the timeout for waiting for the launched powershell process. (mitigation)
2: Ensure each uses a unique name for the error and done files to avoid polluting the next test.