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

Add table/layout tests #285

Merged
merged 4 commits into from Jan 9, 2019

Conversation

Projects
None yet
3 participants
@hughbe
Copy link
Contributor

hughbe commented Jan 2, 2019

And fix bad debug asserts and fix #283 and fix #284
Fixes #290 and Fixes #291

@hughbe hughbe requested a review from dotnet/dotnet-winforms as a code owner Jan 2, 2019

@hughbe hughbe force-pushed the hughbe:table-tests branch from 0bb51fc to ed8c065 Jan 2, 2019

@zsd4yr
Copy link
Member

zsd4yr left a comment

Hey @hughbe reviewing your changes here. First, thanks for writing these!

I am going to request these changes throughout but in general, I'd like you to:

  • add ClassName_ to the front of all these tests. I realize this is tedious but it really makes looking at the results much easier
  • for any tests which take inline primitive or string data (ints, floats, etc...) see if you can use the providers in TestHelpers instead of providing your own arbitrary values. This will make it easier to increase our coverage by changing the test helpers instead of changing each instance of inline data. (for example, your test headed on line 178 in DockPaddingEdgesTests)
  • consider removing ternarys from your asserts as these checks appear to be non-deterministic. You should either write two tests which check for either scenario (? as well as :) or, less ideally, remove the check. I won't block for this but it would be really nice to have.

Again, thanks 😄 and let me know if you'd like any additionally clarification / want to discuss some of these points

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 2, 2019

Also noticing that you could be using Mocks via Moq in a large variety of cases instead of extending your own class just for testing. This is our preferred method

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 3, 2019

add ClassName_ to the front of all these tests. I realize this is tedious but it really makes looking at the results much easier

I'm not sure I agree with this. The test class itself describes what we're testing

Take the following example. It's fairly obvious that there's a failure in MenuItem tests. In fact, surely adding another suffix MenuItemTests.MenuItem_Ctor_String_MenuItemArray actually makes it harder to see what failed and where?

[xUnit.net 00:00:01.29]     System.Windows.Forms.MenuItemTests.Ctor_String_MenuItemArray(text: "value", items: [System.Windows.Forms.MenuItem, Items.Count: 0, Text: ]) [FAIL]
Failed   System.Windows.Forms.MenuItemTests.Ctor_String_MenuItemArray(text: "value", items: [System.Windows.Forms.MenuItem, Items.Count: 0, Text: ])
Error Message:
 Assert.False() Failure
Expected: False
Actual:   True
Stack Trace:
   at System.Windows.Forms.MenuItemTests.Ctor_String_MenuItemArray(String text, MenuItem[] items) in /Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms.Design/tests/System/Windows/Forms/MenuItemTests.cs:line 199

In terms of the other changes, I'm not free for the next couple of days on holiday but I agree some stuff can be moved to TestHelpers or Moq and I'll remove or add parameters for the ternaries - I don't want to seem to be testing the implementation by simply copying conditions into the tests!

@zsd4yr
Copy link
Member

zsd4yr left a comment

add ClassName_ to the front of all these tests. I realize this is tedious but it really makes looking at the results much easier

I'm not sure I agree with this. The test class itself describes what we're testing

Unfortunately, our current testing infrastructure only displays results according to the name of the method, and does not include the class name of the object being tested. I understand your concerns, but for ease of viewing results, we need to do it this way for now.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 3, 2019

In terms of the other changes, I'm not free for the next couple of days on holiday but I agree some stuff can be moved to TestHelpers or Moq and I'll remove or add parameters for the ternaries - I don't want to seem to be testing the implementation by simply copying conditions into the tests!

Sounds great 😄 enjoy your holiday, and I'll eagerly await the remaining requested changes.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 4, 2019

@hughbe after a discussion with the team today, we are asking you to separate this into three PRs, (1) which fixes those bugs you found, another (2) which cleans up these debugs, and a final (3) which contains the tests.

Please let me know if you'd like any clarification about this decision.

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 4, 2019

@zsd4yr sure - it makes more sense considering this PR is getting big and the comments are basically untrackable!

@hughbe hughbe force-pushed the hughbe:table-tests branch 2 times, most recently from 7357e07 to adf2ff0 Jan 4, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 4, 2019

Let's be sure to rerun the CI once those tests fixes get in

/// We cannot inherit from MemberDataAttribute as it is sealed, so we have to reimplement
/// ConvertDataItem inheriting from MemberDataAttributeBase.
/// </summary>
public class CommonMemberDataAttribute : MemberDataAttributeBase

This comment has been minimized.

@sharwell

sharwell Jan 4, 2019

Member

Nice, this worked every bit as well as I was hoping.

💡 sealed

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 5, 2019

@hughbe when you get a chance, can you rebase this against master now that your #294 is merged? 😄 then we should be able to rerun the CI and pass these tests

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 5, 2019

#293 needs to be merged first :)

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

@hughbe done

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

@AdamYoblick can you check out these tests?

@hughbe hughbe force-pushed the hughbe:table-tests branch from fec5fc9 to 0295cc3 Jan 7, 2019


using Xunit;

namespace System.Windows.Forms.Tests

This comment has been minimized.

@zsd4yr

zsd4yr Jan 9, 2019

Member

why was this deleted?

This comment has been minimized.

@hughbe

hughbe Jan 9, 2019

Contributor

Renamed to PanelTests.cs

This comment has been minimized.

@zsd4yr

zsd4yr Jan 9, 2019

Member

works for me

@zsd4yr

zsd4yr approved these changes Jan 9, 2019

@zsd4yr zsd4yr merged commit 7e45f20 into dotnet:master Jan 9, 2019

1 check passed

license/cla All CLA requirements met.
Details
@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 9, 2019

:shipit: thanks for all the work @hughbe

@hughbe hughbe deleted the hughbe:table-tests branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment