Small refactoring on dapr client and invocation handler. Add improvements on test structure. #1379
Conversation
…ents on test structure. Signed-off-by: Rafael Camara <rafaelcamarac@gmail.com>
Signed-off-by: Rafael Camara <rafaelcamarac@gmail.com>
RafaelJCamara
force-pushed
the
improvements-dapr-client
branch
from
a973203
to
ac635b3
Compare
There was a problem hiding this comment.
Thank you for your pull request. I appreciate the effort you’ve put into it. However, I noticed several changes that seem unnecessary, particularly the renaming of arguments without a clear reason. As mentioned in my comments, we prefer to avoid such changes unless they are essential, as our XML documentation already provides clarity.
If a new feature requires renaming for better functionality or readability, that’s understandable. However, changes should not be made solely for the sake of change. Please revert these specific changes, and in future pull requests, ensure that any renaming is justified by a significant need.
| @@ -79,47 +83,10 @@ public void TryRewriteUri_FailsForRelativeUris() | |||
| } | |||
|
|
|||
| [Theory] | |||
| [InlineData(null, "http://bank", "https://some.host:3499/v1.0/invoke/bank/method/")] | |||
There was a problem hiding this comment.
I disagree with removing all these tests - again, they're each verifying a unique and separate variation of the inputs that is worth knowing is covered.
There was a problem hiding this comment.
I'm not removing them, I just placed them in the class data TryRewriteUriWithNoAppIdRewritesUriToDaprInvokeTestData and all of them are being called here.
The reason why I've done this was to make this testing class a bit more cleaner.
If you don't agree with moving the test data to that class, I can revert this change.
There was a problem hiding this comment.
Which I've now seen you want to revert back to the attributes.
| @@ -0,0 +1,56 @@ | |||
| using Xunit; | |||
There was a problem hiding this comment.
Please put these back with the inline data attributes. When building unit tests, I don't expect to have to look in a separate file to add additional variations, I expect to see them inline using the convenience attributes provided by the framework. This might make sense in an actual library, but not in a unit test class.
| @@ -98,63 +96,68 @@ internal string? DaprApiToken | |||
| /// <inheritdoc /> | |||
| protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | |||
| { | |||
| var original = request.RequestUri; | |||
| if (!this.TryRewriteUri(request.RequestUri, out var rewritten)) | |||
| var originalUri = request.RequestUri; | |||
There was a problem hiding this comment.
I'm disinclined to accept this change which only changes the inline variable. If you're rewriting the method to do something and you need to change the variable so it's more meaningful, by all means, but to change it for the sake of changing it isn't necessary.
| request.Headers.Remove("dapr-api-token"); | ||
| } | ||
| } | ||
|
|
||
| // Internal for testing | ||
| internal bool TryRewriteUri(Uri? uri, [NotNullWhen(true)] out Uri? rewritten) | ||
| internal bool TryRewriteUri(Uri? uri, [NotNullWhen(true)] out Uri? rewrittenUri) |
There was a problem hiding this comment.
Same thing - I'm disinclined to accept this change that renames the argument just because you can. Again, if you're changing the method itself and need to change the name so it's useful in your rewrite, that's one thing, but changing it just for the sake of doing so here, especially for something that's strictly internal for testing purposes, doesn't make sense.
| @@ -29,11 +29,7 @@ public void CreateInvokeHttpClient_WithAppId() | |||
| [Fact] | |||
| public void CreateInvokeHttpClient_InvalidAppId() | |||
| { | |||
| var ex = Assert.Throws<ArgumentException>(() => | |||
| { | |||
| // The appId needs to be something that can be used as hostname in a URI. | |||
There was a problem hiding this comment.
I think it's useful to retain this comment in the test as it provides more context to the test name and what it's covering
|
@WhitWaldo I was looking at the codebase to understand how somethings worked and I thought it was a good idea to suggest some improvements. I see that most of my suggested changes are proposed for revert. Do you think it still makes sense to have the rest of the changes up or shall we have put this PR to sleep? |
Thank you for your follow-up. I appreciate your understanding regarding the previous points. Several of the remaining changes in this PR appear to be formatting-based (e.g., tab inserts) or minor syntax updates (like converting a get body to a lambda expression). Generally, I would prefer not to accept these types of changes unless they are part of a larger feature addition. I do see value in the following change in this PR: var ex = Assert.Throws<ArgumentException>(() =>
{
_ = DaprClient.CreateInvokeHttpClient(daprEndpoint: "ftp://localhost:3500");
});to var ex = Assert.Throws<ArgumentException>(() => DaprClient.CreateInvokeHttpClient(daprEndpoint: "ftp://localhost:3500"));...and the others like it. If you could revert the other changes and keep this one, I'd be happy to accept this PR. For future reference however, I'd prefer that PRs include more substantial feature additions or changes rather than consisting solely of small formatting and refactoring changes. |
|
I'll close this one and perhaps open another one with such improvements. Thanks a lot! |
Description
Issue reference
No issues to be referenced.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: