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

WIP: Add DesignerUtils, ControlDesigner.get_SelectionRules & DesignerActionListsChangedEventArgs Tests #2376

Open
wants to merge 10 commits into
base: master
from

Conversation

@brucremo
Copy link

brucremo commented Nov 16, 2019

Issue #721 (Partial) :

Added tests for the following classes:

  • System/Windows/Forms/Design/DesignerUtils.cs (4 tests to be fixed for Moq assemblies)
  • System/ComponentModel/Design/DesignerActionListsChangedEventArgs.cs

And methods:

  • System.Windows.Forms.Design.ControlDesigner.get_SelectionRules
Microsoft Reviewers: Open in CodeFlow
@brucremo brucremo requested a review from dotnet/dotnet-winforms as a code owner Nov 16, 2019
@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Nov 16, 2019

CLA assistant check
All CLA requirements met.

#pragma warning disable CS0657
[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
#pragma warning restore CS0657
Comment on lines 177 to 179

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 17, 2019

Member

Why do we need this?

This comment has been minimized.

Copy link
@brucremo

brucremo Nov 21, 2019

Author

This code is meant to enable Moq to mock sealed classes like ToolboxSnapDragDropEventArgs, but apparently this is not working. We also have the same code on System.Windows.Forms.InternalsVisibleTo.cs, which I believe was created for the same purpose, so the test was skipped for the moment.

This comment has been minimized.

Copy link
@weltkante

weltkante Nov 22, 2019

Contributor

Just for the record, there are better ways to do this, but maybe your test framework isn't smart enough to use them.

  • when dynamically generating code on the fly you can set the owner and/or set a flag to disable visibility checks and automatically get access to internals, so no need to do InternalsVisibleTo
  • otherwise adding IgnoresAccessChecksTo in the referencing assembly is better than adding InternalsVisibleTo in the referenced assembly (needs a nuget package if you compile with VS/Roslyn instead of dynamically generating code)

IMHO adding new InternalsVisibleTo is bad and should be avoided, unless you have it explicitely on the long term plan of how your test infrastructure should look like in the WinForms project

This comment has been minimized.

Copy link
@brucremo

brucremo Nov 22, 2019

Author

Thanks for the feedback and knowledge @weltkante! I will improve the tests based on the information to fix this pull request

@brucremo

This comment has been minimized.

Copy link
Author

brucremo commented Nov 25, 2019

Based on @weltkante's comments and some research I have checked the way that Moq accesses internals and the possibility to mock the sealed classes ToolboxSnapDragDropEventArgs and DesignerOptionCollection, used and required by the following tests, which are currently skipped:

I believe there could be a better way of doing this and I could be wrong, or testing these methods and creating the mocks for them incorrectly, but while working and searching for ways to fix these problems and avoid the skips I found out a few things:

  • Moq can't in any way mock sealed classes, it is not supported using Dynamic Proxy Generation.

  • The projects that have unit tests already have their internals set to visible for their tests by using InternalsVisibleTo through the InternalsVisibleTo.cs file under src\Properties folder. This approach works, exposing internal types to the Dynamic Proxy Generation assembly, but it does not resolve the problem that Moq cannot mock sealed classes.

  • To test the DesignerUtils methods mentioned before, these classes have to be mocked somehow since the methods do not accept null objects.

Based on these findings, and coming from the principle that the Winforms project uses Moq as its main testing framework for mocks, I was thinking there are a few options to solve this problem and future issues on tests that might require mocking with sealed classes using Moq (or not):

  • There is a way of wrapping these required classes or mocking them in a better way, following the concepts that other (paid) testing frameworks on this specific scenario, like IL rewriting.

  • These methods should not have unit tests, because I might be overthinking them and this is not the correct way of implementation.

If anyone could guide me on how to proceed with these cases I would be very thankful! I wish to continue to contribute to Winforms and learn as much as I can!

Thanks in advance!

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Nov 26, 2019

We had internal discussions with @AdamYoblick and @JeremyKuhne about ways to test different aspects of code.
We considered a number of different options (such as an introduction of interfaces, the TestAccessor pattern, reflection etc) and arrived to a conclusion that we would employ reflection to access members that are no readily accessible via public API surface (e.g. privates).
We should use the same approach for sealed types (since you can't subclass them).

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Nov 26, 2019

I wish to continue to contribute to Winforms and learn as much as I can!

Thank you! 👍
We are all learning along the way, and that's the best part of it

@RussKie RussKie changed the title Issue #721 (Partial) : Add DesignerUtils, ControlDesigner.get_SelectionRules & DesignerActionListsChangedEventArgs Tests WIP: Add DesignerUtils, ControlDesigner.get_SelectionRules & DesignerActionListsChangedEventArgs Tests Nov 26, 2019
@AdamYoblick

This comment has been minimized.

Copy link
Member

AdamYoblick commented Nov 26, 2019

Yep, here's Jeremy's example of the reflection solution: https://gist.github.com/JeremyKuhne/e8c9c33d037ac5e5ed4c477817cae414

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Nov 27, 2019

Thank you Adam, I knew I saw an example somewhere!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.