Skip to content

Commit e783298

Browse files
cominterncomintern
committed
Address review comments, test threading issues.
1 parent 355d459 commit e783298

File tree

8 files changed

+87
-57
lines changed

8 files changed

+87
-57
lines changed

‎Rubberduck.Core/UI/UnitTesting/TestExplorerViewModel.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,7 @@ private void ExecuteCopyResultsCommand(object parameter)
307307
}
308308
}
309309

310+
// TODO - FIXME
310311
//KEEP THIS, AS IT MAKES FOR THE BASIS OF A USEFUL *SUMMARY* REPORT
311312
//private void ExecuteCopyResultsCommand(object parameter)
312313
//{

‎Rubberduck.Parsing/UIContext/UiDispatcher.cs

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ public void FlushMessageQueue()
4646
{
4747
CheckInitialization();
4848

49+
if (_contextProvider.UiContext == SynchronizationContext.Current)
50+
{
51+
PumpMessages();
52+
}
53+
else
54+
{
55+
InvokeAsync(PumpMessages);
56+
}
57+
58+
// This should remain a local function - messages should not be pumped out outside of FlushMessageQueue.
4959
void PumpMessages()
5060
{
5161
var message = new NativeMethods.NativeMessage();
@@ -59,15 +69,6 @@ void PumpMessages()
5969

6070
handle.Free();
6171
}
62-
63-
if (_contextProvider.UiContext == SynchronizationContext.Current)
64-
{
65-
PumpMessages();
66-
}
67-
else
68-
{
69-
InvokeAsync(PumpMessages);
70-
}
7172
}
7273

7374
private const uint RPC_E_SERVERCALL_RETRYLATER = 0x8001010A;

‎Rubberduck.UnitTesting/UnitTesting/TestEngine.cs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ internal class TestEngine : ITestEngine
4545
public bool CanRun => AllowedRunStates.Contains(_state.Status) && _vbe.IsInDesignMode;
4646
public bool CanRepeatLastRun => _lastRun.Any();
4747

48-
private bool _refreshBackoff;
48+
private bool _listening = true;
4949

5050
public TestEngine(
5151
RubberduckParserState state,
@@ -76,12 +76,12 @@ private void StateChangedHandler(object sender, ParserStateEventArgs e)
7676

7777
if (!CanRun || e.IsError)
7878
{
79-
_refreshBackoff = false;
79+
_listening = true;
8080
}
8181
// CanRun returned true already, only refresh tests if we're not backed off
82-
else if (!_refreshBackoff && e.OldState != ParserState.Busy)
82+
else if (_listening && e.OldState != ParserState.Busy)
8383
{
84-
_refreshBackoff = true;
84+
_listening = false;
8585
var updates = TestDiscovery.GetAllTests(_state).ToList();
8686
var run = new List<TestMethod>();
8787
var known = new Dictionary<TestMethod, TestOutcome>();
@@ -118,12 +118,14 @@ private void OnTestRunStarted(IReadOnlyList<TestMethod> tests)
118118
{
119119
CancellationRequested = false;
120120
TestRunStarted?.Invoke(this, new TestRunStartedEventArgs(tests));
121+
// This call is safe - OnTestRunStarted cannot be called from outside RD's context.
121122
_uiDispatcher.FlushMessageQueue();
122123
}
123124

124125
private void OnTestStarted(TestMethod test)
125126
{
126127
TestStarted?.Invoke(this, new TestStartedEventArgs(test));
128+
// This call is safe - OnTestStarted cannot be called from outside RD's context.
127129
_uiDispatcher.FlushMessageQueue();
128130
}
129131

@@ -133,6 +135,7 @@ private void OnTestCompleted(TestMethod test, TestResult result)
133135
_knownOutcomes.Add(test, result.Outcome);
134136

135137
TestCompleted?.Invoke(this, new TestCompletedEventArgs(test, result));
138+
// This call is safe - OnTestCompleted cannot be called from outside RD's context.
136139
_uiDispatcher.FlushMessageQueue();
137140
}
138141

@@ -275,6 +278,9 @@ private void RunWhileSuspended(IEnumerable<TestMethod> tests)
275278
continue;
276279
}
277280

281+
// The message pump is flushed here to catch cancellation requests. This is the only place inside the main test running
282+
// loop where this is "safe" to do - any other location risks either potentially misses test teardown or risks not knowing
283+
// what teardown needs to be done via VBA.
278284
_uiDispatcher.FlushMessageQueue();
279285

280286
if (CancellationRequested)

‎Rubberduck.UnitTesting/UnitTesting/TestResult.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
namespace Rubberduck.UnitTesting
44
{
5-
public struct TestResult
5+
public readonly struct TestResult
66
{
77
public TestResult(TestOutcome outcome, string output = "", long duration = 0)
88
{

‎RubberduckTests/UnitTesting/EngineTests.cs

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55
using System;
66
using System.Collections.Generic;
77
using System.Linq;
8+
using System.Threading;
89
using Rubberduck.Parsing.VBA;
910

1011
namespace RubberduckTests.UnitTesting
1112
{
12-
[TestFixture]
13+
[NonParallelizable]
14+
[TestFixture, Apartment(ApartmentState.STA)]
1315
public class EngineTests
1416
{
1517
[Test]
@@ -155,33 +157,33 @@ private void SetupAndTestAssertAndReturn(Action action, TestResult expected)
155157
}
156158

157159
engine.TestEngine.Run(engine.TestEngine.Tests);
160+
Thread.SpinWait(25);
158161

159162
Mock.Verify(engine.Dispatcher, engine.VbeInteraction, engine.WrapperProvider, engine.TypeLib);
160163
Assert.AreEqual(1, completionEvents.Count);
161164
Assert.AreEqual(expected, completionEvents.First().Result);
162165
}
163166
}
164167

165-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
166168
private static readonly Dictionary<TestOutcome, (TestOutcome Outcome, string Output, long Duration)> DummyOutcomes = new Dictionary<TestOutcome, (TestOutcome, string, long)>
167169
{
168170
{ TestOutcome.Succeeded, (TestOutcome.Succeeded, "", 0) },
169171
{ TestOutcome.Inconclusive, (TestOutcome.Inconclusive, "", 0) },
170172
{ TestOutcome.Failed, (TestOutcome.Failed, "", 0) },
171-
//{ TestOutcome.SpectacularFail, (TestOutcome.SpectacularFail, "", 0) },
173+
{ TestOutcome.SpectacularFail, (TestOutcome.SpectacularFail, "", 0) },
172174
{ TestOutcome.Ignored, (TestOutcome.Ignored, "", 0) }
173175
};
174176

175177
[Test]
178+
[NonParallelizable]
176179
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Failed })]
177180
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Succeeded, TestOutcome.Succeeded })]
178181
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Inconclusive, TestOutcome.Failed })]
179182
[TestCase(new object[] { TestOutcome.Inconclusive, TestOutcome.Inconclusive, TestOutcome.Inconclusive })]
180183
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
181184
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
182185
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.Ignored, TestOutcome.Ignored })]
183-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
184-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
186+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
185187
[Category("Unit Testing")]
186188
public void TestEngine_LastTestRun_UpdatesAfterRun(params TestOutcome[] tests)
187189
{
@@ -190,11 +192,14 @@ public void TestEngine_LastTestRun_UpdatesAfterRun(params TestOutcome[] tests)
190192
using (var engine = new MockedTestEngine(underTest))
191193
{
192194
engine.TestEngine.Run(engine.TestEngine.Tests);
195+
Thread.SpinWait(25);
196+
193197
Assert.AreEqual(underTest.Count, engine.TestEngine.LastRunTests.Count);
194198
}
195199
}
196200

197201
[Test]
202+
[NonParallelizable]
198203
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Failed })]
199204
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Succeeded, TestOutcome.Succeeded })]
200205
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Inconclusive, TestOutcome.Failed })]
@@ -221,6 +226,8 @@ public void TestEngine_RunByOutcome_RunsAppropriateTests(params TestOutcome[] te
221226
completionEvents.Clear();
222227
engine.TestEngine.RunByOutcome(outcome);
223228

229+
Thread.SpinWait(25);
230+
224231
var expected = tests.Count(result => result == outcome);
225232
Assert.AreEqual(expected, completionEvents.Count);
226233

‎RubberduckTests/UnitTesting/MockedTestEngine.cs

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -160,25 +160,38 @@ public void SetupAssertCompleted(TestMethod testMethod, TestResult result)
160160
// there is no STA boundary being crossed, so the internal implementation of the AssertHandler is not constrained by
161161
// the rental context that would normally be managed by Interop. These tests *do* pass as of the commit that adds them,
162162
// but only if the context is force by running them through the debugger. Leaving these in as commented code mainly for
163-
// the purpose of documenting this. Single asserts are fine.
164-
165-
//case TestOutcome.SpectacularFail:
166-
// action = () =>
167-
// {
168-
// for (var failure = 0; failure < 10; failure++)
169-
// {
170-
// AssertHandler.OnAssertFailed(result.Output, testMethod.Declaration.IdentifierName);
171-
// }
172-
// };
173-
// break;
163+
// the purpose of documenting this. Single asserts are fine. Update - added spin wait instead. This may still be a FIXME?
164+
165+
case TestOutcome.SpectacularFail:
166+
action = () =>
167+
{
168+
var assert = new AssertClass();
169+
for (var failure = 0; failure < 10; failure++)
170+
{
171+
assert.Fail(result.Output);
172+
}
173+
};
174+
break;
174175
case TestOutcome.Failed:
175-
action = () => AssertHandler.OnAssertFailed(result.Output, testMethod.Declaration.IdentifierName);
176+
action = () =>
177+
{
178+
var assert = new AssertClass();
179+
assert.Fail(result.Output);
180+
};
176181
break;
177182
case TestOutcome.Inconclusive:
178-
action = () => AssertHandler.OnAssertInconclusive(result.Output);
183+
action = () =>
184+
{
185+
var assert = new AssertClass();
186+
assert.Inconclusive(result.Output);
187+
};
179188
break;
180189
case TestOutcome.Succeeded:
181-
action = AssertHandler.OnAssertSucceeded;
190+
action = () =>
191+
{
192+
var assert = new AssertClass();
193+
assert.Succeed();
194+
};
182195
break;
183196
default:
184197
action = () => { };

‎RubberduckTests/UnitTesting/TestExplorerModelTests.cs

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System.Collections.Generic;
22
using System.Linq;
3+
using System.Threading;
34
using System.Windows.Media;
45
using Moq;
56
using NUnit.Framework;
@@ -8,7 +9,8 @@
89

910
namespace RubberduckTests.UnitTesting
1011
{
11-
[TestFixture]
12+
[NonParallelizable]
13+
[TestFixture, Apartment(ApartmentState.STA)]
1214
public class TestExplorerModelTests
1315
{
1416
[Test]
@@ -45,13 +47,12 @@ public void ExecutedCount_MatchesNumberOfTestsRun(int testCount)
4547

4648
private const int DummyTestDuration = 10;
4749

48-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
4950
private static readonly Dictionary<TestOutcome, (TestOutcome Outcome, string Output, long Duration)> DummyOutcomes = new Dictionary<TestOutcome, (TestOutcome, string, long)>
5051
{
5152
{ TestOutcome.Succeeded, (TestOutcome.Succeeded, "", DummyTestDuration) },
5253
{ TestOutcome.Inconclusive, (TestOutcome.Inconclusive, "", DummyTestDuration) },
5354
{ TestOutcome.Failed, (TestOutcome.Failed, "", DummyTestDuration) },
54-
//{ TestOutcome.SpectacularFail, (TestOutcome.SpectacularFail, "", DummyTestDuration) },
55+
{ TestOutcome.SpectacularFail, (TestOutcome.SpectacularFail, "", DummyTestDuration) },
5556
{ TestOutcome.Ignored, (TestOutcome.Ignored, "", DummyTestDuration) }
5657
};
5758

@@ -63,8 +64,7 @@ public void ExecutedCount_MatchesNumberOfTestsRun(int testCount)
6364
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
6465
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
6566
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.Ignored, TestOutcome.Ignored })]
66-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
67-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
67+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
6868
[Category("Unit Testing")]
6969
public void LastTestSucceededCount_CountIsCorrect(params TestOutcome[] tests)
7070
{
@@ -74,6 +74,7 @@ public void LastTestSucceededCount_CountIsCorrect(params TestOutcome[] tests)
7474
{
7575
model.Engine.ParserState.OnParseRequested(model);
7676
model.Model.ExecuteTests(model.Model.Tests);
77+
Thread.SpinWait(25);
7778

7879
var expected = tests.Count(outcome => outcome == TestOutcome.Succeeded);
7980
Assert.AreEqual(expected, model.Model.LastTestSucceededCount);
@@ -88,8 +89,7 @@ public void LastTestSucceededCount_CountIsCorrect(params TestOutcome[] tests)
8889
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
8990
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
9091
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.Ignored, TestOutcome.Ignored })]
91-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
92-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
92+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
9393
[Category("Unit Testing")]
9494
public void LastTestIgnoredCount_CountIsCorrect(params TestOutcome[] tests)
9595
{
@@ -99,6 +99,7 @@ public void LastTestIgnoredCount_CountIsCorrect(params TestOutcome[] tests)
9999
{
100100
model.Engine.ParserState.OnParseRequested(model);
101101
model.Model.ExecuteTests(model.Model.Tests);
102+
Thread.SpinWait(25);
102103

103104
var expected = tests.Count(outcome => outcome == TestOutcome.Ignored);
104105
Assert.AreEqual(expected, model.Model.LastTestIgnoredCount);
@@ -113,8 +114,7 @@ public void LastTestIgnoredCount_CountIsCorrect(params TestOutcome[] tests)
113114
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
114115
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
115116
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.Ignored, TestOutcome.Ignored })]
116-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
117-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
117+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
118118
[Category("Unit Testing")]
119119
public void LastTestInconclusiveCount_CountIsCorrect(params TestOutcome[] tests)
120120
{
@@ -124,6 +124,7 @@ public void LastTestInconclusiveCount_CountIsCorrect(params TestOutcome[] tests)
124124
{
125125
model.Engine.ParserState.OnParseRequested(model);
126126
model.Model.ExecuteTests(model.Model.Tests);
127+
Thread.SpinWait(25);
127128

128129
var expected = tests.Count(outcome => outcome == TestOutcome.Inconclusive);
129130
Assert.AreEqual(expected, model.Model.LastTestInconclusiveCount);
@@ -138,8 +139,7 @@ public void LastTestInconclusiveCount_CountIsCorrect(params TestOutcome[] tests)
138139
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
139140
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
140141
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.Ignored, TestOutcome.Ignored })]
141-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
142-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
142+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
143143
[Category("Unit Testing")]
144144
public void LastTestFailedCount_CountIsCorrect(params TestOutcome[] tests)
145145
{
@@ -149,6 +149,7 @@ public void LastTestFailedCount_CountIsCorrect(params TestOutcome[] tests)
149149
{
150150
model.Engine.ParserState.OnParseRequested(model);
151151
model.Model.ExecuteTests(model.Model.Tests);
152+
Thread.SpinWait(25);
152153

153154
var expected = tests.Count(outcome => outcome == TestOutcome.Failed);
154155
Assert.AreEqual(expected, model.Model.LastTestFailedCount);
@@ -162,9 +163,8 @@ public void LastTestFailedCount_CountIsCorrect(params TestOutcome[] tests)
162163
[TestCase(new object[] { TestOutcome.Inconclusive, TestOutcome.Inconclusive, TestOutcome.Inconclusive })]
163164
[TestCase(new object[] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
164165
[TestCase(new object[] { TestOutcome.Succeeded, TestOutcome.Ignored })]
165-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
166-
//[TestCase(new object[] { TestOutcome.SpectacularFail, TestOutcome.SpectacularFail, TestOutcome.Ignored })]
167-
//[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
166+
[TestCase(new object[] { TestOutcome.SpectacularFail, TestOutcome.SpectacularFail, TestOutcome.Ignored })]
167+
[TestCase(new object[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
168168
[Category("Unit Testing")]
169169
public void LastTestSpectacularFailCount_CountIsCorrect(params TestOutcome[] tests)
170170
{
@@ -174,6 +174,7 @@ public void LastTestSpectacularFailCount_CountIsCorrect(params TestOutcome[] tes
174174
{
175175
model.Engine.ParserState.OnParseRequested(model);
176176
model.Model.ExecuteTests(model.Model.Tests);
177+
Thread.SpinWait(25);
177178

178179
var expected = tests.Count(outcome => outcome == TestOutcome.SpectacularFail);
179180
Assert.AreEqual(expected, model.Model.LastTestSpectacularFailCount);
@@ -201,7 +202,7 @@ public void CancelTestRun_RequestsCancellation()
201202
{ "Gold", Colors.Gold },
202203
{ "Orange", Colors.Orange },
203204
{ "Red", Colors.Red },
204-
//{ "Black", Colors.Black }
205+
{ "Black", Colors.Black }
205206
};
206207

207208
[Test]
@@ -212,8 +213,7 @@ public void CancelTestRun_RequestsCancellation()
212213
[TestCase("Gold", new [] { TestOutcome.Inconclusive, TestOutcome.Inconclusive, TestOutcome.Succeeded })]
213214
[TestCase("Red", new [] { TestOutcome.Failed, TestOutcome.Failed, TestOutcome.Failed })]
214215
[TestCase("Orange", new [] { TestOutcome.Succeeded, TestOutcome.Ignored })]
215-
// See comment in MockedTestEngine.SetupAssertCompleted re. commented code.
216-
//[TestCase("Black", new [] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
216+
[TestCase("Black", new[] { TestOutcome.Ignored, TestOutcome.SpectacularFail })]
217217
[Category("Unit Testing")]
218218
public void ProgressBarColor_CorrectGivenTestResult(params object[] args)
219219
{
@@ -226,6 +226,7 @@ public void ProgressBarColor_CorrectGivenTestResult(params object[] args)
226226
{
227227
model.Engine.ParserState.OnParseRequested(model);
228228
model.Model.ExecuteTests(model.Model.Tests);
229+
Thread.SpinWait(25);
229230

230231
Assert.AreEqual(ColorLookup[expectedColor], model.Model.ProgressBarColor);
231232
}

0 commit comments

Comments
 (0)