Skip to content

Use powershell sleep in unit tests#6332

Merged
benvillalobos merged 2 commits intomainfrom
dev/kirillo/sleep
Apr 22, 2021
Merged

Use powershell sleep in unit tests#6332
benvillalobos merged 2 commits intomainfrom
dev/kirillo/sleep

Conversation

@KirillOsenkov
Copy link
Copy Markdown
Member

Fixes #6265

The sleep NuGet package (https://www.nuget.org/packages/sleep) was uploaded to the dotnet-public feed for us by Matt Mitchell.

The source is here: https://github.com/KirillOsenkov/Misc/tree/main/Sleep

This makes our build less dependent on the machine state. Currently on Windows it may pick up C:\Program Files\Git\usr\bin\sleep.exe if it is installed and on the PATH (this is what happens on CI). But for users that don't have it on the PATH the test fails. Let's make sure our build is hermetic and self-sufficient.

Also renaming a method parameter to add clarity.

Fixes #6265

The sleep NuGet package (https://www.nuget.org/packages/sleep) was uploaded to the dotnet-public feed for us by Matt Mitchell.

The source is here: https://github.com/KirillOsenkov/Misc/tree/main/Sleep

This makes our build less dependent on the machine state. Currently on Windows it may pick up C:\Program Files\Git\usr\bin\sleep.exe if it is installed and on the PATH (this is what happens on CI). But for users that don't have it on the PATH the test fails. Let's make sure our build is hermetic and self-sufficient.

Also renaming a method parameter to add clarity.
Copy link
Copy Markdown
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Thanks! I appreciate this.

@ladipro
Copy link
Copy Markdown
Member

ladipro commented Apr 9, 2021

This looks great although I wonder if the extra dependency is worth it if we can simply make the test run cmd /c timeout 600 on Windows.

@KirillOsenkov
Copy link
Copy Markdown
Member Author

lol NOW you show up!

I had no idea this exists:

C:\>where timeout
C:\Windows\System32\timeout.exe

C:\>timeout /?

TIMEOUT [/T] timeout [/NOBREAK]

Description:
    This utility accepts a timeout parameter to wait for the specified
    time period (in seconds) or until any key is pressed. It also
    accepts a parameter to ignore the key press.

Parameter List:
    /T        timeout       Specifies the number of seconds to wait.
                            Valid range is -1 to 99999 seconds.

    /NOBREAK                Ignore key presses and wait specified time.

    /?                      Displays this help message.

NOTE: A timeout value of -1 means to wait indefinitely for a key press.

Examples:
    TIMEOUT /?
    TIMEOUT /T 10
    TIMEOUT /T 300 /NOBREAK
    TIMEOUT /T -1

Sigh I'm just too lazy to redo it. Maybe I should.

@KirillOsenkov
Copy link
Copy Markdown
Member Author

@ladipro I've switched to timeout.exe which is in C:\Windows\system32, thanks for the tip!

@KirillOsenkov
Copy link
Copy Markdown
Member Author

Unfortunately when I tried to use timeout.exe on CI it failed with exit code 1. I have no idea why :-/

I force pushed my original approach with a custom NuGet package which seems to work both locally and on CI.

If anyone wants to investigate the timeout approach, you can use the commit b9957ef

Copy link
Copy Markdown
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

@KirillOsenkov apologies for randomizing you. No idea why timeout wouldn't work in CI and not worth spending time on it for sure. Thank you!

@KirillOsenkov
Copy link
Copy Markdown
Member Author

No problem at all, I did learn something. I'm guessing timeout won't work when there is no active user session logged on (just a guess though). Other than that I have no idea why it wouldn't work.

@rainersigwald
Copy link
Copy Markdown
Member

We've solved this in the past with no external dependencies. Let me see how that works and try to use it here.

@rainersigwald
Copy link
Copy Markdown
Member

There it is:

/// <summary>
/// Gets a command template that can be used by an Exec task to sleep for the specified amount of time. The string has to be formatted with the number of seconds to sleep
/// </summary>
internal static string GetSleepCommandTemplate()
{
return
NativeMethodsShared.IsWindows
? "@powershell -NoLogo -NoProfile -command &quot;Start-Sleep -Milliseconds {0}&quot; &gt;nul"
: "sleep {0}";
}

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 16, 2021
@KirillOsenkov
Copy link
Copy Markdown
Member Author

Nice, thanks for fixing this for me.

@benvillalobos benvillalobos changed the title Deploy sleep.exe to use with unit-tests Use powershell sleep in unit tests Apr 22, 2021
@benvillalobos benvillalobos merged commit 10d6a76 into main Apr 22, 2021
@benvillalobos benvillalobos deleted the dev/kirillo/sleep branch April 22, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProcessExtensions_Tests.KillTree() test failure

5 participants