8000 Drop the `Shuffle` convenience method. by plioi · Pull Request #314 · fixie/fixie · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Drop the Shuffle convenience method. #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2023
Merged

Drop the Shuffle convenience method. #314

merged 1 commit into from
Dec 11, 2023

Conversation

plioi
Copy link
Contributor
@plioi plioi commented Dec 11, 2023 •
edited
Loading

Rarely used or recommended, the Shuffle convenience method was provided for three reasons:

  1. During v1 development, a short-term goal was to be able to use Fixie conventions to mimic the behavior of similar frameworks like xUnit and NUnit, so it was added to ease mimicking xUnit's test order behavior specifically. Framework mimicry is no longer an explicit goal, now that the practice served its purpose of uncovering requirements.
  2. There was no such routine available in the framework, and having end users write it by hand was subtle enough that it'd risk introducing quiet bugs that would amount to skipping tests. You'd know something was wrong, but the evidence would be far from the cause.
  3. Writing your own would involve directly using some Random instance, which has a long history of thread safety issues where the behavior and proper solution varied greatly from .NET Framework, to .NET Core/5, to .NET 6+. Bottling up the recommendation in a convenience method hid the use of Random while using it safely.

.NET 6 introduced Random.Shared for unambiguous thread safety.

.NET 8 introduced Random.Shuffle<T>(...) for the in-place shuffling of a span or array.

The extension method easily misled end users to perform shuffling during the Discovery phase, instead of the Execution phase, which isn't wrong but would easily lead to only shuffling test methods within a class, for instance, while the test class order remained fixed, without being apparent to the developer. Users will be better off altering test order in their Execution phase where the full impact of their implementation will be more clear.

Now that we target .NET 8+, we have no reason to continue maintaining or suggesting this code. Users can call Random.Shared.Shuffle(...) and ideally in their IExecution implementation.

@plioi
Drop the Shuffle convenience method.
deb545c 
1. The capability is provided directly by the framework since .NET 6.

2. We no longer have a pressing concern about end users making thread-unsafe attempts at using `Random`, which was likely before .NET 6 introduced `Random.Shared`.
@plioi plioi marked this pull request as ready for review December 11, 2023 04:22
@plioi plioi merged commit ea97e94 into main Dec 11, 2023
@plioi plioi deleted the unshuffle branch December 11, 2023 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@plioi
0