Skip to content

Commit

Permalink
Update CountdownEventTest.cs
Browse files Browse the repository at this point in the history
  • Loading branch information
@WojciechNagorski
WojciechNagorski committed Apr 14, 2023
1 parent 7b2b39b commit a9f5c1d
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/Renci.SshNet.Tests/Classes/Common/CountdownEventTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,10 @@ public void WaitHandle_WaitOne_ShouldReturnFalseWhenTimeoutExpiresBeforeCountdow
Assert.IsFalse(actual);
Assert.IsFalse(countdownEvent.IsSet);
Assert.IsFalse(countdownEvent.WaitHandle.WaitOne(0));
Assert.IsTrue(watch.Elapsed >= timeout);
// Use precision because it fail in CI/CD pipeline on .NET 7. Locally it worked without precision.
// But I think it is not supper important because this is internal .NET implementation.
var precision = TimeSpan.FromMilliseconds(6);
Assert.IsTrue(watch.Elapsed >= timeout.Subtract(precision));

This comment has been minimized.

Sign in to view

Copy link
@zybexXL

zybexXL Apr 14, 2023 •
edited
Loading

Contributor

This is not acceptable, it's faking the timer to get to a desired result.

This entire testcase is wrong and prone to race conditions. It starts up to 20 threads, and then starts measuring time. But it fails to account for the time it takes to start the threads; by the time the last thread is started, the first one might have already completed (or be almost complete), so the 30ms timeout may in fact be a sufficient delay for the event to be signaled. The 20 thread execution will also depend on whether this runs on a 4/8/16/32 core machine - the 32-core machine can start the 20 threads much faster than the 4-core one.

Also, what exactly is this test testing? I would just throw it away if it's just testing .Net intrinsics.

countdownEvent.Wait(Session.InfiniteTimeSpan);
countdownEvent.Dispose();
Expand Down

0 comments on commit a9f5c1d

Please sign in to comment.