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

Upgrade FluentAssertions package #918

Merged

Conversation

dotnetspark
Copy link
Contributor

@dotnetspark dotnetspark commented Feb 12, 2022

The issue or feature being addressed

#913

Details on the issue fix or feature implementation

Upgrading FluentAssertions package to latest

Confirm the following

  • I started this PR by branching from the head of the latest dev vX.Y branch, or I have rebased on the latest dev vX.Y branch, or I have merged the latest changes from the dev vX.Y branch
  • I have targeted the PR to merge into the latest dev vX.Y branch as the base branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@dnfadmin
Copy link

dnfadmin commented Feb 12, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up @ylr-research!

Other than the one comment, these changes all look reasonable.

src/Polly.Specs/Registry/ReadOnlyPolicyRegistrySpecs.cs Outdated Show resolved Hide resolved
@martincostello martincostello added this to the v7.3.0 milestone Feb 12, 2022
@martincostello martincostello linked an issue Feb 12, 2022 that may be closed by this pull request
@martincostello
Copy link
Member

The tests seem to be hanging somewhere.

@dotnetspark
Copy link
Contributor Author

dotnetspark commented Feb 12, 2022

@martincostello, I just wrapped the kvp into a dictionary so it could compile on my end. I can't see the tests results.

@martincostello
Copy link
Member

I would guess the problem is with one of the asynchronous tests you've changed, and the previous failure was masking it.

@dotnetspark
Copy link
Contributor Author

I see 673 passed but it doesn't say which one failed

@martincostello
Copy link
Member

I think that's the problem, it hasn't failed, it's still running and has hung.

@dotnetspark
Copy link
Contributor Author

Do you think re-running could yield a different result? Otherwise, any suggestion on how to repro the one(s) hanging

@martincostello
Copy link
Member

Not without going away and researching dotnet test parameters for helping to find such tests.

Presumably these all pass when you run them locally using the build script?

@dotnetspark
Copy link
Contributor Author

I missed that. I was just compiling in VS.

1 similar comment
@dotnetspark
Copy link
Contributor Author

I missed that. I was just compiling in VS.

@dotnetspark
Copy link
Contributor Author

I missed that. I was just compiling in VS. Let me check that

@dotnetspark
Copy link
Contributor Author

I missed that. I was just building in VS. Let me try that

@dotnetspark
Copy link
Contributor Author

@martincostello I found the issues. 30 tests not passing to be precise. All related to the same. Since I have changed Throw by ThrowAsync the expected result instead of 2 is now 1. Doing so let the test pass. Is this change correct?
Take test Should_wait_asynchronously_for_async_onretry_delegate for example, line 395 executeDelegateInvocations.Should().Be(1);

Await all usages of Awaiting().
@martincostello
Copy link
Member

martincostello commented Feb 13, 2022

Thanks for your contribution @ylr-research - I cloned the PR locally and took a look at the code.

The fundamental issue that was causing the deadlocks was that all the usages of Awaiting() needed to be awaited, so all the tests using it needed to be made async Task. I've gone through every single usage of the method and done the conversion, and now everything passes as expected.

@martincostello martincostello merged commit 9d7d8bc into App-vNext:v724-or-v730 Feb 13, 2022
@martincostello martincostello modified the milestones: v7.3.0, v7.2.4 Feb 13, 2022
@dotnetspark
Copy link
Contributor Author

Great, thanks

@dotnetspark dotnetspark deleted the Upgrade_FluentAssertions branch February 13, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to FluentAssertions 6.x.x
3 participants