Fix race condition in tests #7819

iSazonov · 2018-09-19T11:22:21Z

PR Summary

Last days the tests often failed due to race condition.
See https://powershell.visualstudio.com/PowerShell/_build/results?buildId=2022&view=logs

Fix is to use test hook (write verbose message at cmdlet start) and Wait-UntilTrue function.

PR Checklist

TravisEz13 · 2018-09-19T17:09:23Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Xml.Tests.ps1

these is no guarantee that this will take a significant about of time. It may finish before you stop it.

@JamesWTruher Can you review?

these is no guarantee

Yes, I'd remove the tests - I do not see a way to make these tests reliable. If we just add a hook with an infinite loop...

The intent of the test is to cover the StopProcessing code path as identified by code coverage. I wonder if we can just have at test hook in the cmdlet so that ProcessRecord() doesn't return if the test hook is in place. Therefore we can call Stop() so that the StopProcessing code path gets exercised?

Seems like the implementation would be for ProcessRecord to Sleep for enough time that we are certain enough that we can stop it. This could also mean that we can get one object and process it instead of trying to process a lot of records trying to make it take a long time.

Test hook added.
Sleep suspend current thread. So I used only short sleep to slow down a cmdlet.

TravisEz13 · 2018-09-19T17:13:37Z

test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1

I have similar concerns here about test reliability

iSazonov · 2018-09-20T13:52:44Z

Reopen to restart Ci Appveyor.

iSazonov · 2018-09-20T15:17:33Z

Reopen to restart Ci Appveyor.

TravisEz13 · 2018-09-20T21:47:27Z

@iSazonov Do you have a Microsoft account? I can give you permissions to restart the Azure Pipelines if you give me a Microsoft account. feel free to send it to Steve or my work email.

iSazonov · 2018-09-21T03:34:23Z

@TravisEz13 I sent.

SteveL-MSFT · 2018-09-21T22:12:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs

I don't think this sleep is needed. There was a concern that the cmdlet would finish successfully before the stop of the pipeline got issued. I think they way to solve that is to have another check at the end of ProcessRecord() where if the test hook is enabled, just have it sleep and not finish.

Stopping doesn't work on sleeping runspace. So idea is only slow down the cycle and make many interations.

Ok, perhaps add a comment?

SteveL-MSFT · 2018-09-24T18:00:35Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs

Ok, perhaps add a comment?

JamesWTruher

Generally, I'm in favor of more test hooks, but I'm not a fan of extra verbosity. My only real objection is the hard coding of the timeout duration. I think that should be testhook supplied value.

JamesWTruher · 2018-09-25T21:46:16Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs

I would be happier if this value was in the testhook (ala TestStopComputerResults). This would enable us to change the duration easily, and even specific to a particular test.

JamesWTruher · 2018-09-25T21:48:17Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs

what are you trying to achieve with the WriteVerbose? I'm not sure there's much value here and I would rather reduce the chatty output.

The verbose message here is used to tell the test that the cmdlet has started so that it can issue the stop. Otherwise there's a window where stop is issued before the cmdlet under test has started.

Yes, we need an event to start the stopping.
(We could use another test hook for the flag.)

JamesWTruher · 2018-09-25T21:57:00Z

test/tools/Modules/HelpersCommon/HelpersCommon.psm1

As this is a general purpose helper, I'm wondering whether this should be | Should -Not -Be "Running"
some of the states might be "failed" or "disconnected".

If the stopping doesn't work we can get "Completed".

iSazonov · 2018-10-01T05:27:36Z

@SteveL-MSFT Please update your review.

SteveL-MSFT · 2018-10-01T22:50:31Z

test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1

Isn't it better to use:

$sb = { 1..10000 | Export-CliXml -Path $testfile -Verbose }

This will give syntax coloring and can be analyzed by psscriptanalyzer in vscode

SteveL-MSFT · 2018-10-03T00:43:07Z

test/powershell/Modules/Microsoft.PowerShell.Utility/XMLCommand.Tests.ps1

Another one to change to { ... }

Sorry. Fixed.

SteveL-MSFT

The test failures indicates that the pipeline finishes before stop is processed. Perhaps we need to rethink how to make the stop predictable.

stale · 2018-11-02T16:48:12Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

iSazonov · 2018-11-06T06:16:40Z

No ideas have I how make it more stable so close.

iSazonov requested a review from SteveL-MSFT September 19, 2018 11:22

iSazonov requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners September 19, 2018 11:22

iSazonov requested a review from adityapatwardhan September 19, 2018 11:22

iSazonov self-assigned this Sep 19, 2018

iSazonov requested a review from TravisEz13 September 19, 2018 11:38

TravisEz13 reviewed Sep 19, 2018

View reviewed changes

iSazonov mentioned this pull request Sep 19, 2018

Add EscapeHandling parameter in ConvertTo-Json cmdlet #7775

Merged

11 tasks

iSazonov requested review from BrucePay, anmenaga and daxian-dbw as code owners September 20, 2018 07:38

iSazonov closed this Sep 20, 2018

iSazonov reopened this Sep 20, 2018

iSazonov closed this Sep 20, 2018

iSazonov reopened this Sep 20, 2018

SteveL-MSFT requested changes Sep 21, 2018

View reviewed changes

SteveL-MSFT requested changes Sep 24, 2018

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs Outdated

Copy link

Member

SteveL-MSFT Sep 24, 2018

Choose a reason for hiding this comment

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

Ok, perhaps add a comment?

iSazonov force-pushed the fix-race-condition-in-tests branch from 861e0c2 to dddfaa2 Compare September 25, 2018 04:36

JamesWTruher reviewed Sep 25, 2018

View reviewed changes

SteveL-MSFT requested changes Oct 1, 2018

View reviewed changes

SteveL-MSFT requested changes Oct 3, 2018

View reviewed changes

iSazonov force-pushed the fix-race-condition-in-tests branch from 4f9a224 to 67b656a Compare October 3, 2018 03:08

Convert tabs to spaces in tests

20b4e73

iSazonov added 12 commits October 3, 2018 18:22

Use test hook in XML commands

0d8b026

Use test hook

2e5fb8d

Suppress unneeded StyleCop message

b855bc9

Use async BeginStop()

680f1a0

Create Test-Stopping() function

fc0291f

Fix typo and slow down Import-CliXml

e3e0778

Add comment

a81629c

Convert test hook to int type

3e51c92

Add SlowDownCmdletInMilliseconds parameter in Test-Stopping

80c742a

Fix typo

c9ee507

Address feedback

057edb5

Address feedback 2

7507fd7

iSazonov force-pushed the fix-race-condition-in-tests branch from 67b656a to 7507fd7 Compare October 3, 2018 13:22

Enlarge test file

3503a05

SteveL-MSFT approved these changes Oct 3, 2018

View reviewed changes

SteveL-MSFT requested changes Oct 3, 2018

View reviewed changes

stale bot added the Stale label Nov 2, 2018

iSazonov closed this Nov 6, 2018

iSazonov deleted the fix-race-condition-in-tests branch May 15, 2020 13:09

Fix race condition in tests #7819

Fix race condition in tests #7819

Uh oh!

Conversation

iSazonov commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 20, 2018

Uh oh!

iSazonov commented Sep 20, 2018

Uh oh!

TravisEz13 commented Sep 20, 2018

Uh oh!

iSazonov commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

stale bot commented Nov 2, 2018

Uh oh!

iSazonov commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

iSazonov commented Sep 19, 2018 •

edited

Loading

iSazonov Sep 26, 2018 •

edited

Loading