Skip to content

Upgrade FluentAssertions package#918

Merged
martincostello merged 3 commits into
App-vNext:v724-or-v730from
dotnetspark:Upgrade_FluentAssertions
Feb 13, 2022
Merged

Upgrade FluentAssertions package#918
martincostello merged 3 commits into
App-vNext:v724-or-v730from
dotnetspark:Upgrade_FluentAssertions

Conversation

@dotnetspark

@dotnetspark dotnetspark commented Feb 12, 2022

Copy link
Copy Markdown
Contributor

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

dnfadmin commented Feb 12, 2022

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@martincostello martincostello left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/Polly.Specs/Registry/ReadOnlyPolicyRegistrySpecs.cs Outdated
@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
Copy Markdown
Member

The tests seem to be hanging somewhere.

@dotnetspark

dotnetspark commented Feb 12, 2022

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@martincostello

Copy link
Copy Markdown
Member

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

@dotnetspark

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

I missed that. I was just compiling in VS.

1 similar comment
@dotnetspark

Copy link
Copy Markdown
Contributor Author

I missed that. I was just compiling in VS.

@dotnetspark

Copy link
Copy Markdown
Contributor Author

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

@dotnetspark

Copy link
Copy Markdown
Contributor Author

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

@dotnetspark

Copy link
Copy Markdown
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);

Yadel Lopez and others added 2 commits February 13, 2022 02:34
Await all usages of Awaiting().
@martincostello

martincostello commented Feb 13, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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