Skip to content
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

Unit tests missing for MutexSecurity, SemaphoreSecurity, EventWaitHandleSecurity #43346

Open
carlossanlop opened this issue Oct 13, 2020 · 7 comments
Assignees
Labels
area-System.Threading test-enhancement Improvements of test source code
Milestone

Comments

@carlossanlop
Copy link
Member

Currently we only have one unit test file testing MutexSecurity, and contains only one unit test that verifies the parameterless constructor:

namespace System.Security.AccessControl
{
public class MutexSecurityTests
{
[Fact]
public void Ctor_Success()
{
new MutexSecurity();
}
}
}

We also need test classes to verify SemaphoreSecurity and EventWaitHandleSecurity as well.

@carlossanlop carlossanlop added area-System.Threading test-enhancement Improvements of test source code help wanted [up-for-grabs] Good issue for external contributors labels Oct 13, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 13, 2020
@eugene-shcherbo
Copy link
Contributor

Hi, would you mind me working on this? I would love to pick it up.

@danmoseley danmoseley removed untriaged New issue has not been triaged by the area owner help wanted [up-for-grabs] Good issue for external contributors labels Oct 13, 2020
@danmoseley
Copy link
Member

Hi @eugene-shcherbo yes, for sure - I assigned to you.

@eugene-shcherbo
Copy link
Contributor

Thank you @danmosemsft

@eugene-shcherbo
Copy link
Contributor

eugene-shcherbo commented Oct 15, 2020

Hi @carlossanlop, @danmosemsft . I wonder if we want to cover with tests internal methods of the mentioned classes. I'm asking because now the assembly with code seems not to expose its internal members to the tests assembly. Therefore I would gladly discuss with you (when you have time) what you think of it and if we should do something with this.

My opinion is that it would be good to test them, at least the tests would be like an additional documentation, but I'm not sure what issues addding the InternalsVisibleToAttribute attribute can cause, so looking forward to your opinions or advice, guys. Thanks!

@danmoseley
Copy link
Member

@carlossanlop is the code owner. I can say more generally: we try to test only through public API and we should do that if at all possible. Amongst other reasons that makes it easier to refactor the product code. In the rare instances where it isn't good enough, it is possible to use InternalsvisibleToAttribute but compiling selected product source files into the test assembly is preferable. This is the approach taken to test the Uri class. But we should make every effort to test through public API.

@eugene-shcherbo
Copy link
Contributor

eugene-shcherbo commented Oct 15, 2020

@danmosemsft I get the idea, makes sense actually. Thank you for the answer, this is very useful to know. And apologizing for mentioning you in the question especially if I distracted you.

@danmoseley
Copy link
Member

You are welcome to mention me any time!

@mangod9 mangod9 added this to the 6.0.0 milestone Oct 16, 2020
@kouvel kouvel modified the milestones: 6.0.0, 7.0.0 Jul 30, 2021
@mangod9 mangod9 modified the milestones: 7.0.0, Future Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Threading test-enhancement Improvements of test source code
Projects
None yet
Development

No branches or pull requests

6 participants