Fix a ton of potential crashers, odd code and redundant calls in ModelService #1450
Conversation
| Assert.AreEqual(3, fetched.Count); | ||
| Assert.AreEqual("snoopy", fetched[0].Login); | ||
| Assert.AreEqual("github", fetched[1].Login); | ||
| Assert.AreEqual("fake", fetched[2].Login); |
There was a problem hiding this comment.
The Assert.That constraint model is preferred for nunit 3 apparently. See #1411 (comment) and the surrounding comments for context on this.
There was a problem hiding this comment.
Ah, ok. It's just that it inverts the order of the parameters, makes it even more confusing than usual. I'll fix these then.
There was a problem hiding this comment.
Yeah, I think we need to switch the order of parameters in quite a few tests if we want to be technically correct. However, it doesn't actually impact the testing and it's tedious work...
|
Also, I think the fix for |
10786d1 to
648d67c
Compare
| await repositories.OriginalCompleted; | ||
|
|
||
| // the first time we do this, it should fetch from the API | ||
| apiClient.Received().GetRepositories(); |
There was a problem hiding this comment.
This is warning that the result is not awaited. Need to add await here.
| repositories.Subscribe(); | ||
| await repositories.OriginalCompleted; | ||
| // the second time we do this, it should not fetch | ||
| apiClient.DidNotReceive().GetRepositories(); |
Conflicts: test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs
`apiClient.GetRepositories()` is `async` so an `await` needs to be added to the `Received()`/`DidNotReceive` calls to prevent compiler warnings.
Line 155 (`apiClient.GetUser().Returns(Observable.Return(CreateOctokitUser("snoopy")));`) should be doing this.
The ordering of `expectedRepos` vs `repositories` is not important.
Ugh, what a mess. While fixing #1319, I came across a bunch of repository tests that were not actually using the code that the app is using.
Moved those tests to use the real API and deleted the old deprecated ModelService methods, and in the process fixed an old
TrackingCollectionbug where theOriginalCompletedobservable would finish before all items have been processed.Added exception catching to various places so we don't crash if keys aren't found or the network is down while accessing data.
This needs #1449 for tests to pass.