Merge pull request #1483 from github/fixes/trackingcollection-originalcompleted

shana · web-flow · commit c7576a7e2bce · 2018-02-14T13:43:41.000+01:00
TrackingCollection Fixes
diff --git a/src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs b/src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs
@@ -174,7 +174,8 @@ enum TheAction
             Add,
             Insert,
             Remove,
-            Ignore
+            Ignore,
+            End
         }
 
         bool isChanging;
@@ -211,7 +212,7 @@ enum TheAction
         readonly Dictionary<T, int> filteredIndexCache = new Dictionary<T, int>();
 
         bool originalSourceIsCompleted;
-        bool signalOriginalSourceCompletion;
+        bool sourceHasData;
         ReplaySubject<Unit> originalSourceCompleted;
         public IObservable<Unit> OriginalCompleted => originalSourceCompleted;
 
@@ -287,6 +288,7 @@ public IObservable<T> Listen(IObservable<T> obs)
                 })
                 .Do(data =>
                 {
+                    sourceHasData = true;
                     cache.Enqueue(new ActionData(data));
                     signalHaveData.OnNext(Unit.Default);
                 })
@@ -296,16 +298,15 @@ public IObservable<T> Listen(IObservable<T> obs)
                         return;
 
                     originalSourceIsCompleted = true;
-                    if (!cache.IsEmpty)
+                    if (!sourceHasData)
                     {
-                        signalOriginalSourceCompletion = true;
+                        originalSourceCompleted.OnNext(Unit.Default);
+                        originalSourceCompleted.OnCompleted();
                     }
                     else
                     {
-                        originalSourceCompleted.OnNext(Unit.Default);
-                        originalSourceCompleted.OnCompleted();
-                        signalNeedData.OnCompleted();
-                        signalHaveData.OnCompleted();
+                        cache.Enqueue(new ActionData(TheAction.End, null));
+                        signalHaveData.OnNext(Unit.Default);
                     }
                 })
                 .Publish();
@@ -321,16 +322,23 @@ public IObservable<T> Listen(IObservable<T> obs)
                 {
                     var delay = CalculateProcessingDelay(interval);
                     waitHandle.Wait(delay);
-                    dataListener.OnNext(GetFromQueue());
+                    var data = GetFromQueue();
+                    if (!data.Equals(ActionData.Default))
+                    {
+                        dataListener.OnNext(data);
+                    }
                     return Unit.Default;
                 })
                 .Publish();
 
             source = dataListener
-                .Where(data => data.Item != null)
+                .Where(data => data.Item != null || data.TheAction == TheAction.End)
                 .ObserveOn(scheduler)
                 .Select(data =>
                 {
+                    if (data.TheAction == TheAction.End)
+                        return data;
+
                     data = ProcessItem(data, original);
 
                     // if we're removing an item that doesn't exist, ignore it
@@ -351,18 +359,15 @@ public IObservable<T> Listen(IObservable<T> obs)
                     data = FilteredRemove(data);
                     return data;
                 })
-                .Do(_ =>
+                .Do(data =>
                 {
-                    if (ManualProcessing)
+                    if (data.TheAction == TheAction.End)
                     {
-                        if (signalOriginalSourceCompletion)
-                        {
-                            signalOriginalSourceCompletion = false;
-                            originalSourceCompleted.OnNext(Unit.Default);
-                            originalSourceCompleted.OnCompleted();
-                        }
+                        originalSourceCompleted.OnNext(Unit.Default);
+                        originalSourceCompleted.OnCompleted();
                     }
-                    else
+
+                    if (!ManualProcessing)
                         signalNeedData.OnNext(Unit.Default);
                 })
                 .Where(data => data.Item != null)
@@ -1174,7 +1179,7 @@ void Reset()
             pumpDisposables.Clear();
             disposables.Clear();
             originalSourceIsCompleted = false;
-            signalOriginalSourceCompletion = false;
+            sourceHasData = false;
             cache = new ConcurrentQueue<ActionData>();
             dataListener = new ReplaySubject<ActionData>();
             disposables.Add(dataListener);
diff --git a/test/GitHub.InlineReviews.UnitTests/Properties/AssemblyInfo.cs b/test/GitHub.InlineReviews.UnitTests/Properties/AssemblyInfo.cs
@@ -1,6 +1,7 @@
 ﻿using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
+using NUnit.Framework;
 
 // General Information about an assembly is controlled through the following 
 // set of attributes. Change these attribute values to modify the information
@@ -34,3 +35,5 @@
 // [assembly: AssemblyVersion("1.0.*")]
 [assembly: AssemblyVersion("1.0.0.0")]
 [assembly: AssemblyFileVersion("1.0.0.0")]
+
+[assembly: Timeout(2 /*minutes*/ * 60 * 1000)]
diff --git a/test/GitHub.InlineReviews.UnitTests/packages.config b/test/GitHub.InlineReviews.UnitTests/packages.config
@@ -10,6 +10,7 @@
   <package id="Microsoft.Win32.Primitives" version="4.0.1" targetFramework="net461" />
   <package id="NSubstitute" version="2.0.3" targetFramework="net461" />
   <package id="NUnit" version="3.9.0" targetFramework="net461" />
+  <package id="NUnit.ConsoleRunner" version="3.7.0" targetFramework="net461" />
   <package id="NUnit3TestAdapter" version="3.9.0" targetFramework="net461" />
   <package id="Rx-Core" version="2.2.5-custom" targetFramework="net461" />
   <package id="Rx-Interfaces" version="2.2.5-custom" targetFramework="net461" />
diff --git a/test/GitHub.UI.UnitTests/GitHub.UI.UnitTests.csproj b/test/GitHub.UI.UnitTests/GitHub.UI.UnitTests.csproj
@@ -38,9 +38,8 @@
       <HintPath>..\..\packages\NSubstitute.2.0.3\lib\net45\NSubstitute.dll</HintPath>
       <Private>True</Private>
     </Reference>
-    <Reference Include="nunit.framework, Version=3.6.1.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL">
-      <HintPath>..\..\packages\NUnit.3.6.1\lib\net45\nunit.framework.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="nunit.framework, Version=3.9.0.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL">
+      <HintPath>..\..\packages\NUnit.3.9.0\lib\net45\nunit.framework.dll</HintPath>
     </Reference>
     <Reference Include="PresentationCore" />
     <Reference Include="PresentationFramework" />
diff --git a/test/GitHub.UI.UnitTests/Properties/AssemblyInfo.cs b/test/GitHub.UI.UnitTests/Properties/AssemblyInfo.cs
@@ -1,6 +1,7 @@
 ﻿using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
+using NUnit.Framework;
 
 // General Information about an assembly is controlled through the following 
 // set of attributes. Change these attribute values to modify the information
@@ -34,3 +35,5 @@
 // [assembly: AssemblyVersion("1.0.*")]
 [assembly: AssemblyVersion("1.0.0.0")]
 [assembly: AssemblyFileVersion("1.0.0.0")]
+
+[assembly: Timeout(2 /*minutes*/ * 60 * 1000)]
diff --git a/test/GitHub.UI.UnitTests/packages.config b/test/GitHub.UI.UnitTests/packages.config
@@ -1,7 +1,7 @@
 ﻿<?xml version="1.0" encoding="utf-8"?>
 <packages>
   <package id="NSubstitute" version="2.0.3" targetFramework="net461" />
-  <package id="NUnit" version="3.6.1" targetFramework="net461" />
+  <package id="NUnit" version="3.9.0" targetFramework="net461" />
   <package id="NUnit.ConsoleRunner" version="3.7.0" targetFramework="net461" />
   <package id="NUnit3TestAdapter" version="3.9.0" targetFramework="net461" />
   <package id="Serilog" version="2.5.0" targetFramework="net461" />
diff --git a/test/TrackingCollectionTests/ListenerCollectionTests.cs b/test/TrackingCollectionTests/ListenerCollectionTests.cs
@@ -9,7 +9,7 @@
 public class ListenerCollectionTests : TestBase
 {
 #if !DISABLE_REACTIVE_UI
-    [TestFixtureSetUp]
+    [OneTimeSetUp]
     public void Setup()
     {
         Splat.ModeDetector.Current.SetInUnitTestRunner(true);
diff --git a/test/TrackingCollectionTests/Properties/AssemblyInfo.cs b/test/TrackingCollectionTests/Properties/AssemblyInfo.cs
@@ -1,6 +1,7 @@
 ﻿using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
+using NUnit.Framework;
 
 // General Information about an assembly is controlled through the following 
 // set of attributes. Change these attribute values to modify the information
@@ -34,3 +35,5 @@
 // [assembly: AssemblyVersion("1.0.*")]
 [assembly: AssemblyVersion("1.0.0.0")]
 [assembly: AssemblyFileVersion("1.0.0.0")]
+
+[assembly: Timeout(2 /*minutes*/ *60*1000)]
diff --git a/test/TrackingCollectionTests/TrackingCollectionTests.cs b/test/TrackingCollectionTests/TrackingCollectionTests.cs
@@ -18,8 +18,10 @@
 [TestFixture]
 public class TrackingTests : TestBase
 {
+    const int Timeout = 2000;
+
 #if !DISABLE_REACTIVE_UI
-    [TestFixtureSetUp]
+    [OneTimeSetUp]
     public void Setup()
     {
         Splat.ModeDetector.Current.SetInUnitTestRunner(true);
@@ -1907,7 +1909,7 @@ public async Task MultipleSortingAndFiltering()
         col.NewerComparer = OrderedComparer<Thing>.OrderByDescending(x => x.UpdatedAt).Compare;
         col.Subscribe();
 
-        await col.OriginalCompleted;
+        await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
 
         // it's initially sorted by date, so id list should not match
         CollectionAssert.AreNotEqual(list1.Select(x => x.Number).ToEnumerable(), list2.Select(x => x.Number).ToEnumerable());
@@ -1944,7 +1946,7 @@ public async Task MultipleSortingAndFiltering()
     }
 
     [Test]
-    public void ListeningTwiceWorks()
+    public async Task ListeningTwiceWorks()
     {
         var count = 10;
         ITrackingCollection<Thing> col = new TrackingCollection<Thing>();
@@ -1954,13 +1956,15 @@ public void ListeningTwiceWorks()
         var list1 = new List<Thing>(Enumerable.Range(1, count).Select(i => GetThing(i, i, count - i, "Run 1")).ToList());
         var list2 = new List<Thing>(Enumerable.Range(1, count).Select(i => GetThing(i, i, i + count, "Run 2")).ToList());
 
+#pragma warning disable 4014
         col.Listen(list1.ToObservable());
         col.Subscribe();
-        col.OriginalCompleted.Wait();
+        await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
 
         col.Listen(list2.ToObservable());
         col.Subscribe();
-        col.OriginalCompleted.Wait();
+        await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
+#pragma warning restore 4014
 
         CollectionAssert.AreEqual(list2, col);
     }
@@ -1980,7 +1984,7 @@ public void RemovingWithNoObservableSetThrows()
     }
 
     [Test]
-    public async void AddingBeforeSubscribingWorks()
+    public async Task AddingBeforeSubscribingWorks()
     {
         ITrackingCollection<Thing> col = new TrackingCollection<Thing>(Observable.Empty<Thing>());
         ReplaySubject<Thing> done = new ReplaySubject<Thing>();
@@ -1995,7 +1999,7 @@ public async void AddingBeforeSubscribingWorks()
                 done.OnCompleted();
         }, () => {});
 
-        await Observable.Timeout(done, TimeSpan.FromMilliseconds(500));
+        await done.Timeout(TimeSpan.FromMilliseconds(500));
         Assert.AreEqual(2, col.Count);
     }
 
diff --git a/test/TrackingCollectionTests/TrackingCollectionTests.csproj b/test/TrackingCollectionTests/TrackingCollectionTests.csproj
@@ -43,9 +43,8 @@
     <StartupObject />
   </PropertyGroup>
   <ItemGroup>
-    <Reference Include="nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77, processorArchitecture=MSIL">
-      <HintPath>..\..\packages\NUnit.2.6.4\lib\nunit.framework.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="nunit.framework, Version=3.9.0.0, Culture=neutral, PublicKeyToken=2638cd05610744eb, processorArchitecture=MSIL">
+      <HintPath>..\..\packages\NUnit.3.9.0\lib\net45\nunit.framework.dll</HintPath>
     </Reference>
     <Reference Include="System" />
     <Reference Include="System.Reactive.Core, Version=2.2.5.0, Culture=neutral, PublicKeyToken=62aa029873c516b4, processorArchitecture=MSIL">
diff --git a/test/TrackingCollectionTests/packages.config b/test/TrackingCollectionTests/packages.config
@@ -1,6 +1,7 @@
 ﻿<?xml version="1.0" encoding="utf-8"?>
 <packages>
-  <package id="NUnit" version="2.6.4" targetFramework="net452" />
+  <package id="NUnit" version="3.9.0" targetFramework="net461" />
+  <package id="NUnit.ConsoleRunner" version="3.7.0" targetFramework="net461" />
   <package id="NUnit3TestAdapter" version="3.9.0" targetFramework="net461" />
   <package id="Rx-Core" version="2.2.5-custom" targetFramework="net452" />
   <package id="Rx-Interfaces" version="2.2.5-custom" targetFramework="net452" />
diff --git a/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs b/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs
@@ -23,6 +23,7 @@
 
 public class ModelServiceTests
 {
+    const int Timeout = 2000;
     public class TheGetCurrentUserMethod : TestBaseClass
     {
         [Test]
@@ -421,7 +422,7 @@ public async Task NonExpiredIndexReturnsCache()
             col.ProcessingDelay = TimeSpan.Zero;
 
             col.Subscribe();
-            await col.OriginalCompleted;
+            await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));;
 
             Assert.That(expected, Is.EqualTo(col.Count));
             //Assert.Collection(col, col.Select(x => new Action<IPullRequestModel>(t => Assert.That("Cache", StartsWith(x.Title)))).ToArray());
diff --git a/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs
@@ -14,9 +14,12 @@
 using GitHub.Factories;
 using GitHub.Primitives;
 using GitHub.ViewModels.Dialog;
+using System.Diagnostics;
 
 public class RepositoryCloneViewModelTests
 {
+    const int Timeout = 2000;
+
     static RepositoryCloneViewModel GetVM(IModelService modelService, IRepositoryCloneService cloneService, IOperatingSystem os)
     {
         var connection = Substitute.For<IConnection>();
@@ -70,7 +73,7 @@ public async Task LoadsRepositories()
                 Substitute.For<IOperatingSystem>());
 
             var col = (ITrackingCollection<IRemoteRepositoryModel>)vm.Repositories;
-            await col.OriginalCompleted;
+            await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
             Assert.That(3, Is.EqualTo(vm.Repositories.Count));
         }
     }
@@ -102,15 +105,19 @@ public async Task StartsTrueBecomesFalseWhenCompleted()
             repoSubject.OnNext(Substitute.For<IRemoteRepositoryModel>());
             repoSubject.OnNext(Substitute.For<IRemoteRepositoryModel>());
 
-            await done;
+            await done.Timeout(TimeSpan.FromMilliseconds(Timeout));
             done = null;
 
             Assert.True(vm.IsBusy);
 
             repoSubject.OnCompleted();
 
-            await col.OriginalCompleted;
+            await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
 
+            // we need to wait slightly because the subscription OnComplete in the model
+            // runs right after the above await finishes, which means the assert
+            // gets checked before the flag is set
+            await Task.Delay(100);
             Assert.False(vm.IsBusy);
         }
 
@@ -181,8 +188,7 @@ public async Task IsFalseWhenLoadingAndCompletedWithRepository()
             repoSubject.OnCompleted();
 
             var col = (ITrackingCollection<IRemoteRepositoryModel>)vm.Repositories;
-            await col.OriginalCompleted;
-            await Task.Delay(100);
+            await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));
             //Assert.Single(vm.Repositories);
             Assert.False(vm.NoRepositoriesFound);
         }
@@ -206,7 +212,7 @@ public void IsFalseWhenFailed()
         }
 
         [Test]
-        public void IsTrueWhenLoadingCompleteNotFailedAndNoRepositories()
+        public async Task IsTrueWhenLoadingCompleteNotFailedAndNoRepositories()
         {
             var repoSubject = new Subject<IRemoteRepositoryModel>();
             var modelService = Substitute.For<IModelService>();
@@ -221,6 +227,9 @@ public void IsTrueWhenLoadingCompleteNotFailedAndNoRepositories()
 
             repoSubject.OnCompleted();
 
+            // we need to delay slightly because the subscribers listening for OnComplete
+            // need to run before the assert is checked
+            await Task.Delay(100);
             Assert.True(vm.NoRepositoriesFound);
         }
     }
@@ -268,7 +277,7 @@ public void IsFalseIfLoadingReposFails()
         }
 
         [Test]
-        public void IsFalseWhenLoadingCompleteNotFailedAndNoRepositories()
+        public async Task IsFalseWhenLoadingCompleteNotFailedAndNoRepositories()
         {
             var repoSubject = new Subject<IRemoteRepositoryModel>();
             var modelService = Substitute.For<IModelService>();
@@ -283,6 +292,9 @@ public void IsFalseWhenLoadingCompleteNotFailedAndNoRepositories()
 
             repoSubject.OnCompleted();
 
+            // we need to delay slightly because the subscribers listening for OnComplete
+            // need to run before the assert is checked
+            await Task.Delay(100);
             Assert.False(vm.FilterTextIsEnabled);
         }
     }
diff --git a/test/UnitTests/Properties/AssemblyInfo.cs b/test/UnitTests/Properties/AssemblyInfo.cs
@@ -10,4 +10,6 @@
 [assembly: AssemblyDescription("")]
 
 // The following GUID is for the ID of the typelib if this project is exposed to COM
-[assembly: Guid("7c0cb7e5-6c7b-4f11-8454-9e1a4747641c")]
+[assembly: Guid("7c0cb7e5-6c7b-4f11-8454-9e1a4747641c")]
+
+[assembly: Timeout(2 /*minutes*/ *60*1000)]

Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@`
`9`	`9`	`public class ListenerCollectionTests : TestBase`
`10`	`10`	`{`
`11`	`11`	`#if !DISABLE_REACTIVE_UI`
`12`		`- [TestFixtureSetUp]`
	`12`	`+ [OneTimeSetUp]`
`13`	`13`	`public void Setup()`
`14`	`14`	`{`
`15`	`15`	`Splat.ModeDetector.Current.SetInUnitTestRunner(true);`