Replaces FluentAssertions with Shouldly #1768
Conversation
damianh
force-pushed
the
dh/use-shouldly
branch
from
2969c1c to
830683d
Compare
damianh
added
area/identity-server
damianh
force-pushed
the
dh/use-shouldly
branch
4 times, most recently
from
cd77255 to
2957fe5
Compare
damianh
force-pushed
the
dh/use-shouldly
branch
from
2957fe5 to
e79a66b
Compare
damianh
force-pushed
the
dh/use-shouldly
branch
3 times, most recently
from
327675c to
1cc1ee7
Compare
damianh
force-pushed
the
dh/use-shouldly
branch
from
1cc1ee7 to
1c96a20
Compare
There was a problem hiding this comment.
A lot of these comments are nitpicky, such as where we haven't used collection initializers where we do elsewhere. I don't think those necessarily need to be addressed (and could be in a follow-up PR to not keep this open too long). There were some spots it didn't appear to me the new assertions were quite equivalent I think should at least be double checked.
|
|
||
| var responseObject = fields["dto"]; | ||
| responseObject.Should().NotBeNull(); |
There was a problem hiding this comment.
Would leaving this assertion potentially make debugging failures of the test easier? Would it be clearer on a failure to see an assertion failed over a runtime exception if responseObject is null?
|
|
||
| var responseObject = fields["dto"]; | ||
| responseObject.Should().NotBeNull(); |
There was a problem hiding this comment.
Same comment as earlier in the file.
|
|
||
| var responseObject = fields["dto"]; | ||
| responseObject.Should().NotBeNull(); |
|
|
||
| var responseObject = fields["dto"]; | ||
| responseObject.Should().NotBeNull(); |
| _mockPipeline.LoginRequest.LoginHint.Should().Be("login_hint_value"); | ||
| _mockPipeline.LoginRequest.AcrValues.Should().BeEquivalentTo(new string[] { "acr_2", "acr_1" }); | ||
| _mockPipeline.LoginRequest.Parameters.AllKeys.Should().Contain("foo"); | ||
| _mockPipeline.LoginRequest.Parameters.GetValues("foo").Should().BeEquivalentTo(new[] { "foo1", "foo2" }); |
There was a problem hiding this comment.
Are we able to use a collection expression here like two lines above for consistency?
| .Should().NotBeNull() | ||
| .And | ||
| .NotBe(handle); | ||
| .ShouldNotBe(handle); |
There was a problem hiding this comment.
I don't think we've maintained the assertion that the result is not null. This assertion appears it would still pass if assertion was null and it would not be equivalent to handle
| @@ -255,12 +253,12 @@ public async Task UpdateRefreshToken_for_onetime_and_sliding_with_zero_absolute_ | |||
| var refreshToken = await _store.GetRefreshTokenAsync(handle); | |||
| var newHandle = await _subject.UpdateRefreshTokenAsync(new RefreshTokenUpdateRequest { Handle = handle, RefreshToken = refreshToken, Client = client }); | |||
|
|
|||
| newHandle.Should().NotBeNull().And.NotBe(handle); | |||
| newHandle.ShouldNotBe(handle); | |||
There was a problem hiding this comment.
I don't think we've maintained the not null part of the assertion here either.
| result.Audiences.Count.Should().Be(3); | ||
| result.Audiences.Should().BeEquivalentTo(new[] { "api1", "api2", "api3" }); | ||
| result.Audiences.Count.ShouldBe(3); | ||
| result.Audiences.ShouldBe(new[] { "api1", "api2", "api3" }); |
There was a problem hiding this comment.
Collection initializer here as elsewhere?
| _apiCache.Items.Count.Should().Be(2); | ||
| _apiResourceNamesCache.Items.Count.Should().Be(3); | ||
| items.Count().ShouldBe(2); | ||
| items.Select(x => x.Name).ShouldBe(new[] { "foo", "bar" }); |
There was a problem hiding this comment.
Collection initializer here, line 145, line 157, and 170 as done elsewhere?
| result.Resources.ApiScopes.Select(x => x.Name).Should().BeEquivalentTo(new[] { "scope1" }); | ||
| result.Resources.ApiResources.Select(x => x.Name).Should().BeEquivalentTo(new[] { "resource1" }); | ||
| result.Succeeded.ShouldBeTrue(); | ||
| result.Resources.IdentityResources.Select(x => x.Name).ShouldBe(new[] { "openid" }); |
There was a problem hiding this comment.
Collection initializer on these 3 lines, lines 264-266, 306, 411-412, and other spots later in the file?
|
I think Damian was intending to hand this off to us, so go ahead and make any changes you want right on the branch and then we can merge - IMO we don't need still more review after that. |
What issue does this PR address?
Use Should to align with rest of product code base tests.
Added a shared project for shouldly extensions that fills is couple gaps.
Important: Any code or remarks in your Pull Request are under the following terms:
If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.
(see our license, section 7)