Fixing Issues from manual testing #111
Conversation
|
@wli3 I'll add some clarification on which tests are for which bugs, my reasoning behind some of the refactoring, etc tomorrow morning if you want to hold off on reviewing until then. |
|
|
||
| namespace Microsoft.DotNet.Tools.Uninstall.Tests.Shared.IntegrationTests | ||
| { | ||
| public class IntegrationTests |
There was a problem hiding this comment.
Suggestion: The "abstraction level" of this integration test is not good. And in general you should reduce integration test coverage and replace with unit test coverage. The downside of integration tests are (I can think of so far)
- slow
- flaky
- test itself is complicated
- hard to understand why it breaks
- does not encourage better abstraction
- combinations of features require exponential integration tests to cover
to be fair, the upside is,
- if the abstraction layer is right, it is easy for the reader not familiar with the code to understand the test. Like when you can test CLI by passing command line argument and assert the screen output.
- You could have less tests to cover a whole stack, and if you do refactoring, you don't need to edit test. (at the same time, if your tests' level of abstraction is wrong, a small change could cost you to edit all tests, back to the example above, if you changed the output format, out assertion need to be changed)
- Cover integration between "units" easily. (Although it is still possible to test things like dependency injection in unit test)
So a large number of people prefer integration tests. And in reality, a lot of cases are just too hard to put under unit test.
For this specific case, I think the problem is 3, 4. It is complicated in method ListCommandExecutionIsCorrect which means the test itself could be wrong. And for 4, I really cannot tell which part you want to test and if you reproduced the bug in the tests.
I won't block the PR, but you should keep the trade off in mind.
There was a problem hiding this comment.
Thanks for the explanation, that makes sense. The main 2 bugs I found were issues with how methods were called in high level functions (I believe execute functions for some of the commands) so I wasn't sure how to test them, especially with the functional style the tool is written in.
I've refactored a bit in the last update so I can cover the bugs without the confusing integration tests- let me know what you think.
| { | ||
| var match = Regexes.BundleDisplayNameRegex.Match(displayName); | ||
| var cachePathMatch = Regexes.BundleCachePathRegex.Match(bundleCachePath); | ||
| var archString = cachePathMatch.Groups[Regexes.ArchGroupName].Value ?? string.Empty; | ||
| var versionString = cachePathMatch.Groups[Regexes.VersionGroupName].Value; | ||
| var versionFromCachePath = cachePathMatch.Groups[Regexes.VersionGroupName].Value; | ||
| var versionFromRegistry = string.Join('.', (registryKey.GetValue("DisplayVersion") as string).Split('.').Take(3)); |
There was a problem hiding this comment.
could you help me understand this logic? Maybe add some comment, why we can also get the result from CachePath
There was a problem hiding this comment.
Sure, this is essentially the underlying problem with #103 and #106. The cache path for asp.net runtimes doesn't have the version in it so the version can't be parsed out in the same way that it is for the other bundle types. This was causing an error when you tried to list on a machine with these runtimes, since it couldn't be parsed in the expected way.
I'm fixing this here by getting the version from the registry, since this is a more reliable way to get the version for asp.net runtimes.
Are you suggesting adding a comment in the code about this?
There was a problem hiding this comment.
Yes, it is a good place to add comment. It is an external system's behavior and it is not well known at all
There was a problem hiding this comment.
btw there is a way to "document" by test, but it is over kill in this case
Blog post from a friend of mine
https://docs.microsoft.com/en-us/archive/blogs/ericgu/portadaptersimulator-read-only-and-write-only-dependencies
|
@wli3 I made some nontrivial changes based on your feedback about the integration tests- am I still okay to merge this or do you have any other feedback? |
|
Reviewed again, still good to go |
Issues addressed:
--x86option does not display all x86 SDKs #107, Lower patch cannot be uninstalled without--force#108,--all-previewsdoes not uninstall all preview SDKs #109: Fix input of VS version protection calls to ensure consistency--versionoption fails ondotnet-core-uninstall#105: Fix command line structure to support--versionoption