MSTEST0013: AssemblyCleanup should be valid #2353
Conversation
Evangelink
changed the title
| || (!canDiscoverInternals && methodSymbol.GetResultantVisibility() != SymbolVisibility.Public) | ||
| || (canDiscoverInternals && methodSymbol.GetResultantVisibility() == SymbolVisibility.Private)) |
There was a problem hiding this comment.
This will not run.
The declared accessibility of the method should always be public. The DiscoverInternals should only apply to the class (when disabled only public classes with [TestClass] should be scanned, if enabled public and internal classes should be scanned).
There was a problem hiding this comment.
The assembly initialize rule has the same problem:
You will run into this:
@Evangelink this resource, and other resources don't reflect the changes made for Task and ValueTask https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/Resources/Resource.resx#L212
There was a problem hiding this comment.
@nohwnd Yeah that's the correct case here, I added tests that test all those scenarios correctly
There was a problem hiding this comment.
@nohwnd DeclaredAccesibility is what is declared on the method and is the first check Engy's doing here. The other checks are using GetResultantVisibility that walks up tree (class + outer classes) to get the final accessibility. We had offline chat with Engy and the code she wrotes LGTM.
There was a problem hiding this comment.
@Evangelink this resource, and other resources don't reflect the changes made for Task and ValueTask https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/Resources/Resource.resx#L212
Thanks! I see some other resources are missing edit. I'll do a follow-up
There was a problem hiding this comment.
You are right, and I am wrong. :) (For others: The first check makes sure that the class always has public on the member, the other two check the visibility due to placement in a internal or private class (or public class in internal class etc.)).
There was a problem hiding this comment.
@engyebrahim Would you mind doing a follow-up PR where you add a comment on top of the check with what Jakub wrotes for the various analyzers doing the same trick? It will be helpful for other devs.
| || (!canDiscoverInternals && methodSymbol.GetResultantVisibility() != SymbolVisibility.Public) | ||
| || (canDiscoverInternals && methodSymbol.GetResultantVisibility() == SymbolVisibility.Private)) |
There was a problem hiding this comment.
@nohwnd DeclaredAccesibility is what is declared on the method and is the first check Engy's doing here. The other checks are using GetResultantVisibility that walks up tree (class + outer classes) to get the final accessibility. We had offline chat with Engy and the code she wrotes LGTM.
| || (!canDiscoverInternals && methodSymbol.GetResultantVisibility() != SymbolVisibility.Public) | ||
| || (canDiscoverInternals && methodSymbol.GetResultantVisibility() == SymbolVisibility.Private)) |
There was a problem hiding this comment.
@Evangelink this resource, and other resources don't reflect the changes made for Task and ValueTask https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/Resources/Resource.resx#L212
Thanks! I see some other resources are missing edit. I'll do a follow-up
fix: #2230