Skip to content
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.

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

Feature/assertionscope #1091

Merged
merged 17 commits into from Jul 8, 2019

Conversation

@conklinb
Copy link
Contributor

commented Jun 26, 2019

Adds safety to tests using AssertionScope while running many tests in parallel

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.

conklinb added some commits Jun 26, 2019

@conklinb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Looks like the tests need updated to support threading. On it

conklinb added some commits Jun 26, 2019

Show resolved Hide resolved Src/FluentAssertions/FluentAssertions.csproj Outdated
Show resolved Hide resolved Src/FluentAssertions/Execution/AssertionScope.cs Outdated
Show resolved Hide resolved Src/FluentAssertions/Execution/DefaultAssertionStrategy.cs Outdated
Show resolved Hide resolved Src/FluentAssertions/Execution/AssertionScope.cs Outdated
Show resolved Hide resolved Src/FluentAssertions/Execution/AssertionScope.cs Outdated

conklinb added some commits Jun 27, 2019

@conklinb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Sorry about the one file per commit "commits" right now. Work VPN doesn't want to let me log in via bash or gh desktop

@conklinb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

All comments addressed

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

The revised implementation looks great to me 👍.

Now to verification.
I observe that if I remove the [ThreadStatic] from the current implementation the tests fail, whereas this PR succeeds.
So this PR seems at least as good as master.

I've tried to come up with a test to verify that the changes are also an improvement over master.
The test below fails in master, but succeeds with this PR.
If this test like this gets added (with better names than First and Second) I'm confident with these changes.

[Fact]
public async Task When_using_AssertionScope_across_thread_boundaries_it_should_work()
{
    using (var semaphore = new SemaphoreSlim(0, 1))
    {
        await Task.WhenAll(First(semaphore), Second(semaphore));
    }
}

private static async Task First(SemaphoreSlim semaphore)
{
    await Task.Yield();
    var scope = new AssertionScope();
    await semaphore.WaitAsync();
    scope.Should().BeSameAs(AssertionScope.Current);
}

private static async Task Second(SemaphoreSlim semaphore)
{
    await Task.Yield();
    var scope = new AssertionScope();
    semaphore.Release();
    scope.Should().BeSameAs(AssertionScope.Current);
}
Update AssertionScopeSpecs.cs
Adding Tests for boundaries
@conklinb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Added Tests

@jnyrup

jnyrup approved these changes Jul 8, 2019

@conklinb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Is there anything else you need in order to get this merged?

@dennisdoomen dennisdoomen merged commit 1b6c4d3 into fluentassertions:master Jul 8, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jnyrup jnyrup referenced this pull request Jul 26, 2019

Open

Async-safe formatting #1102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.