-
Notifications
You must be signed in to change notification settings - Fork 32
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
[WIP] Refactor tests tryout #73
Conversation
b67ccfc
to
ab37e88
Compare
@@ -201,22 +203,43 @@ public async Task CanAssignToWhenParameterIsTimeSpanAndArgumentIsInvalidString() | |||
rootAndModel.Item2); | |||
} | |||
|
|||
private async static Task<(AttributeArgumentSyntax Syntax, ITypeSymbol Symbol, SemanticModel Model)> GetAttributeSyntaxAsync<T>() |
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.
note: this is just a duplicate for demo purpose.
Please share your thoughts on this refactor, thanks! |
I like the tuples, and I think that we should replace all the old unamed tuples with named tuples throughout the code base (to get rid of Looking at your examples I prefer option 2, but I think that the two examples are only working by mistake (I could be wrong as it is past midnight, and I'm very tired). The reason for why I think that there is a problem is that the class The problem is that we have the following change in the csproj - <Compile Remove="Targets\Extensions\AttributeArgumentSyntaxExtensionsTestsCanAssignToWhenArgumentIsImplicitlyTypedArrayAndNotAssignable.cs" />
+ <Compile Remove="Targets\Extensions\CanAssignToWhenArgumentIsImplicitlyTypedArrayAndNotAssignable.cs" /> but the location of the file is now - in the folder I'll try to take a closer look tomorrow, but now I need to get some sleep. |
So, I finally had time to look at this again. If we correct the csproj, such that I still have the feeling that many of the tests using the files in Of cause we will need some kind of integration tests to see that the entire flow works, but we don't need these integration tests to tests all the corner-cases of the code. For this I would rather have unittests calling For the integration tests I still feel that having the input as part of the test will help when reading and also adding tests. But it can be that I'm trying change too much in one step. |
Superseded by #105 :) |
Wow. I'm too slow 😄. I had started to write that I would close this due to #105, but got sidetracked by a late night snack, and you beat me to it. |
This is a tryout to continue the discussion in #70, with some more concrete examples.
These are 2 directions we could go now, to improve the readability of the tests.
What I have done:
note: this assumes that the class name and the filename are the same. I think that's even best practice.