Fix test decorations

[RussKie]

Microsoft Reviewers: Open in CodeFlow

[weltkante]

I think we discussed this before and as far as I remember I verified that TestData is automatically disposed by xunit, as such you don't have to manually dispose things you get passed as arguments from TestData.

[RussKie]

I think we discussed this before and as far as I remember I verified that TestData is automatically disposed by xunit, as such you don't have to manually dispose things you get passed as arguments from TestData.

I can't recall this, sorry. And this is something I'm trying to work out right now.

I know xUnit serialises (certain?) theories to calculate number of tests, that's why it was impossible to Assert.Same(string, string). But I'm very much confused when it happens and when it doesn't...
Being short on time I haven't had a chance to look inside xUnit implementations, but looking through the signatures I'm not sure how xUnit could dispose a theory parameter.

        public static IEnumerable<object[]> Ctor_Control_DataGridViewCellStyle_TestData()
        {
            yield return new object[] { null, null };
            yield return new object[] { new Button(), new DataGridViewCellStyle() };
        }

        [WinFormsTheory]
        [MemberData(nameof(Ctor_Control_DataGridViewCellStyle_TestData))]
        public void Ctor_Control_DataGridViewCellStyle(Control control, DataGridViewCellStyle cellStyle)
        {
            var e = new DataGridViewEditingControlShowingEventArgs(control, cellStyle);
            Assert.Equal(control, e.Control);
            Assert.Equal(cellStyle, e.CellStyle);

            control?.Dispose();
        }

Here's I know that there are no more uses for the Button.
If the theory data was written like this:

        public static IEnumerable<object[]> Ctor_Control_DataGridViewCellStyle_TestData()
        {
            yield return new object[] { null, null };

			var button = new Button();
            yield return new object[] { button, new DataGridViewCellStyle() };
            yield return new object[] { button { Text = "bla" }, new DataGridViewCellStyle() };
        }

I wouldn't be able to dispose it in the test, because it would affect the 3rd theory data.
How can xUnit know these nuances and to dispose objects correctly?

[weltkante]

I can't recall this, sorry

It was in a PR discussion where it was unclear whether test data needs to be disposed, github doesn't seem to search PR comments so I can't find it again easily, sorry. It was mostly me checking that theory arguments are disposed by design and linking the xunit source which is doing that.

looking through the signatures I'm not sure how xUnit could dispose a theory parameter.

They just check for IDisposable here and disposal happens here. I'm checking for it by making a theory argument disposable and putting a breakpoint into it, then running the test under the debugger.

How can xUnit know these nuances and to dispose objects correctly?

By disposing after running the whole set (and not after each individual test data) they don't have to bother with objects shared across test data. They may call Dispose multiple times on the same object, but that shouldn't hurt since Dispose by design should tolerate multiple calls.

I know xUnit serialises (certain?) theories to calculate number of tests, that's why it was impossible to Assert.Same(string, string). But I'm very much confused when it happens and when it doesn't...

I didn't test disposal under serialization conditions so I don't know how this behaves. However when controls are part of the theory data it would probably just fail because those are not serializable, even though they are MarshalByRef this only should work on Desktop Framework, .NET Core does not support remoting as far as I'm aware. So I wouldn't bother too much, only primitives like strings can be serialized, any complex disposable object will just cause failure and not leak.

If I remember right serialization happens when running tests from VS (which is why you can't just execute all tests in VS but have to select individual ones, since some of the external test runner objects are not serializable). I don't know if its the same serialization you are talking of, there were several different serialization problems, and they may be unrelated.

[RussKie]

I had a chat with @bradwilson and he confirmed the same (implementation and test).

However a new interesting discovery came out as well, that bothered me for a while.

        public static IEnumerable<object[]> Ctor_Control_DataGridViewCellStyle_TestData()
        {
            var button = new Button();
            yield return new object[] { button, new DataGridViewCellStyle() };
            yield return new object[] { button { Text = "bla" }, new DataGridViewCellStyle() };
        }

Brad stated that this code is strictly speaking not legal, and may lead to undefined behaviours. Which means we'll need to remediate the existing code.

/cc: @hughbe @M-Lipin @vladimir-krestov FYI

[hughbe]

Will this fix the hundreds of tests that occasionally fail E.g. https://dev.azure.com/dnceng/public/_build/results?buildId=628388&view=ms.vss-test-web.build-test-results-tab

[RussKie]

Will this fix the hundreds of tests that occasionally fail

I'm expecting to have a more stable test phase. Right now a lot of tests fail because we're running out of available handles 😱

System.ComponentModel.Win32Exception : Error creating window handle.

However I'm observing some additional instabilities that I'm yet to identify - there are too many moving parts - we moved to a different build pool, different build image, updates from the runtime and the arcade, updates to the tests, etc...

[RussKie]

Looks like we had another big deadlock issue with DataGridViewRow tests, see #3209.

[codecov]

Codecov Report

Merging #3064 into master will decrease coverage by 28.88677%.
The diff coverage is 100.00000%.

@@                 Coverage Diff                  @@
##              master       #3064          +/-   ##
====================================================
- Coverage   61.66307%   32.77630%   -28.88677%     
====================================================
  Files           1288         866         -422     
  Lines         451935      254086      -197849     
  Branches       39511       36748        -2763     
====================================================
- Hits          278677       83280      -195397     
+ Misses        167869      166007        -1862     
+ Partials        5389        4799         -590     

Flag	Coverage Δ
#Debug	32.77630% <100.00000%> (-28.88677%)	⬇️
#production	32.77630% <100.00000%> (-0.12791%)	⬇️
#test	?