-
Notifications
You must be signed in to change notification settings - Fork 841
Adopt Polly V8 in Microsoft.Extensions.Http.Resilience #4108
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
Merged
Merged
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
5212ce0
Adopt Polly V8 in Microsoft.Extensions.Http.Resilience
martintmk c71f613
Improve handing of RetryAfter retries
martintmk b41ea69
New tests
martintmk aaabea1
New tests
martintmk fbb819b
Add missing file header
martintmk d15628e
Adopt alpha4
martintmk 58f918a
Cleanup
martintmk 53bc8ac
FIxes
martintmk d85c7bc
Code coverage to 100%
martintmk cf1d47b
Code coverage fix
martintmk 6042436
Fix the build
martintmk b31e661
Attempt to fix code-coverage
martintmk ed761cf
PR comments
martintmk c6e88aa
fix code coverage?
martintmk cd35a19
PR comments and attempt to fix code coverage
martintmk e70559f
Decrease code-coverage
martintmk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
src/Libraries/Microsoft.Extensions.Http.Resilience/FaultInjection/Internal/Log.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 0 additions & 42 deletions
42
src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/HttpClientBuilderExtensions.cs
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have this, the logging code should be tested too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that mean adding exhaustive test coverage for everything the generated logging code does, which is effectively external and an implementation detail?
Seems wasteful to me to 100% cover someone else's code just because it's source-generated as long as it's being exercised by a test. I don't think you'd need it if the requirement wasn't 100% coverage as you could just tweak the threshold, but if it's all or nothing then this seems to be the sensible alternative to exhaustively re-testing the generated code to do logging.
By the same logic I exclude Request Delegate Generator code, otherwise you get fun behaviour like this: dotnet/aspnetcore#48376. I certainly wouldn't spend my time testing ASP.NET Code's error handling scenarios for my user-supplied delegate chasing a coverage metric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Martin says, we had this problem in Polly too. We had to artificially add test cases for code that was auto-generated.
https://github.com/App-vNext/Polly/blob/faaf2ee95b9b92386f8e091277604240f9ab05c0/test/Polly.Extensions.Tests/Telemetry/ResilienceTelemetryDiagnosticSourceTests.cs#L173
Not a big fan of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martincostello That's not what this implies.
This is about ensuring the telemetry produced by this component matches expectations. Within the tests, you use a FakeLogger that captures the telemetry and the ensure that the right thing is coming out of the logging system at the time you expect it to.
As a general rule, the only things that we consider acceptable to exclude from code coverage involve some legacy design patterns that preclude it and some racy code where its impossible to ensure all paths are visited in a given test run. Both are very rare things.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't that custom telemetry logging code called through the compiler generated code? There's no "real" code here in this class, it's just a partial method stub:
extensions/src/Libraries/Microsoft.Extensions.Http.Resilience/FaultInjection/Internal/Log.cs
Lines 13 to 23 in b31e661
It not being covered doesn't preclude the assertions in the fake logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless of course
[LogMethod]doesn't work the same as[LoggerMessage]which I initially thought this was.Looking at some code of my own I have this:
Which gets turned into this:
In that case it's not useful to add tests to test the coverage of the branching for
IsEnabled()on all the logging we have, hence the coverage suppression in my case.If it's a different use case here because that just looks similar to the above then disregard my comment 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Polly we are correctly checking that message is logged:
https://github.com/App-vNext/Polly/blob/faaf2ee95b9b92386f8e091277604240f9ab05c0/test/Polly.Extensions.Tests/Telemetry/ResilienceTelemetryDiagnosticSourceTests.cs#L155
But for some strange reason, it want's us to cover the code-generated code too. For example, the auto-generated struct that backs the entry with all it's properties and methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a bug #4124. In the meantime, I had to manually decrease code-coverage to 98. Once that bug is fixed, we can put it back to 100.