Fix issue 2288

[gabidabet]

Pull Request

The issue or feature being addressed

Fixes #2288.

Details on the issue fix or feature implementation

I used a semaphore slim to control the number of parallel tasks that can run.

Confirm the following

• I started this PR by branching from the head of the default branch
• I have targeted the PR to merge into the default branch
• I have included unit tests for the issue/feature
• I have successfully run a local build

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.38%. Comparing base (22d09c0) to head (df6a367).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2313   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files         313      313           
  Lines        7457     7457           
  Branches     1126     1126           
=======================================
  Hits         6367     6367           
  Misses        745      745           
  Partials      345      345           

Flag	Coverage Δ
linux	85.38% <ø> (+0.02%)	⬆️
macos	85.38% <ø> (ø)
windows	85.38% <ø> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[martincostello]

This seems to resolve the original issue, but I've asked the xunit maintainer who proposed the original workaround to take a look to give their opinion. If we don't hear from them within a week, I'll merge this as-is.

[bradwilson]

Yeah, this seems like a reasonable way to ensure that there is no dependency on the current thread.

The only suggestion I'll make is to change semaphore.Wait(); into one that waits a limited amount of time (long enough to not give false positives, which can be facilitated by putting the test into a test collection that is marked as non-parallelized), and then failing if the wait did not succeed. This will allow the test to wait and fail if somehow the production code stops behaving appropriately, rather than waiting forever (and requiring a "long running tests" detection pass to determine which test is problematic).

[martincostello]

Good suggestion - thanks Brad!

I reckon about 20 seconds should be long enough for our purposes to guard against a "forever wait".

[gabidabet]

Ok I will add a timeout of 20s.