From 29ce27098d1dda1e9a5c62d1676a41bdb42b78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 7 May 2023 17:49:31 +0200 Subject: [PATCH] Refactor again to fix test failures --- .../BeatmapUpdaterMetadataLookupTest.cs | 86 ++++++++++++------- osu.Game/Beatmaps/APIBeatmapMetadataSource.cs | 18 ++-- .../Beatmaps/BeatmapUpdaterMetadataLookup.cs | 16 ++-- .../Beatmaps/IOnlineBeatmapMetadataSource.cs | 10 ++- .../LocalCachedBeatmapMetadataSource.cs | 20 +++-- 5 files changed, 94 insertions(+), 56 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs index 64f44c9922..84195f1e7c 100644 --- a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs +++ b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs @@ -28,9 +28,11 @@ namespace osu.Game.Tests.Beatmaps [Test] public void TestLocalCacheQueriedFirst() { + var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns(new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) + .Returns(true); + apiMetadataSourceMock.Setup(src => src.Available).Returns(true); var beatmap = new BeatmapInfo { OnlineID = 123456 }; @@ -40,19 +42,22 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: false); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); - apiMetadataSourceMock.Verify(src => src.Lookup(It.IsAny()), Times.Never); + localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); + apiMetadataSourceMock.Verify(src => src.TryLookup(It.IsAny(), out It.Ref.IsAny!), Times.Never); } [Test] public void TestAPIQueriedSecond() { + OnlineBeatmapMetadata? localLookupResult = null; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns((OnlineBeatmapMetadata?)null); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) + .Returns(false); + + var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; apiMetadataSourceMock.Setup(src => src.Available).Returns(true); - apiMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns(new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }); + apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out onlineLookupResult)) + .Returns(true); var beatmap = new BeatmapInfo { OnlineID = 123456 }; var beatmapSet = new BeatmapSetInfo(beatmap.Yield()); @@ -61,19 +66,22 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: false); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); - apiMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); + localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); + apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); } [Test] public void TestPreferOnlineFetch() { + var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns(new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) + .Returns(true); + + var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Graveyard }; apiMetadataSourceMock.Setup(src => src.Available).Returns(true); - apiMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns(new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Graveyard }); + apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out onlineLookupResult)) + .Returns(true); var beatmap = new BeatmapInfo { OnlineID = 123456 }; var beatmapSet = new BeatmapSetInfo(beatmap.Yield()); @@ -82,16 +90,18 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: true); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Graveyard)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Never); - apiMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); + localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Never); + apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); } [Test] public void TestPreferOnlineFetchFallsBackToLocalCacheIfOnlineSourceUnavailable() { + var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns(new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) + .Returns(true); + apiMetadataSourceMock.Setup(src => src.Available).Returns(false); var beatmap = new BeatmapInfo { OnlineID = 123456 }; @@ -101,19 +111,22 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: true); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); - apiMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Never); + localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); + apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Never); } [Test] public void TestMetadataLookupFailed() { + OnlineBeatmapMetadata? lookupResult = null; + localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns((OnlineBeatmapMetadata?)null); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out lookupResult)) + .Returns(false); + apiMetadataSourceMock.Setup(src => src.Available).Returns(true); - apiMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns((OnlineBeatmapMetadata?)null); + apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out lookupResult)) + .Returns(true); var beatmap = new BeatmapInfo { OnlineID = 123456 }; var beatmapSet = new BeatmapSetInfo(beatmap.Yield()); @@ -123,8 +136,8 @@ namespace osu.Game.Tests.Beatmaps Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None)); Assert.That(beatmap.OnlineID, Is.EqualTo(-1)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); - apiMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Once); + localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); + apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); } /// @@ -134,11 +147,13 @@ namespace osu.Game.Tests.Beatmaps /// TODO: revisit if/when we have a better flow of queueing metadata retrieval. /// [Test] - public void TestLocalMetadataLookupFailedAndOnlineLookupIsUnavailable([Values] bool preferOnlineFetch) + public void TestLocalMetadataLookupReturnedNoMatchAndOnlineLookupIsUnavailable([Values] bool preferOnlineFetch) { + OnlineBeatmapMetadata? localLookupResult = null; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); - localCachedMetadataSourceMock.Setup(src => src.Lookup(It.IsAny())) - .Returns((OnlineBeatmapMetadata?)null); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) + .Returns(false); + apiMetadataSourceMock.Setup(src => src.Available).Returns(false); var beatmap = new BeatmapInfo { OnlineID = 123456 }; @@ -158,20 +173,25 @@ namespace osu.Game.Tests.Beatmaps /// TODO: revisit if/when we have a better flow of queueing metadata retrieval. /// [Test] - public void TestNoAvailableSources() + public void TestNoAvailableSources([Values] bool preferOnlineFetch) { + OnlineBeatmapMetadata? lookupResult = null; + localCachedMetadataSourceMock.Setup(src => src.Available).Returns(false); + localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out lookupResult)) + .Returns(false); + apiMetadataSourceMock.Setup(src => src.Available).Returns(false); + apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out lookupResult)) + .Returns(false); var beatmap = new BeatmapInfo { OnlineID = 123456 }; var beatmapSet = new BeatmapSetInfo(beatmap.Yield()); beatmap.BeatmapSet = beatmapSet; - metadataLookup.Update(beatmapSet, preferOnlineFetch: false); + metadataLookup.Update(beatmapSet, preferOnlineFetch); Assert.That(beatmap.OnlineID, Is.EqualTo(123456)); - localCachedMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Never); - apiMetadataSourceMock.Verify(src => src.Lookup(beatmap), Times.Never); } } } diff --git a/osu.Game/Beatmaps/APIBeatmapMetadataSource.cs b/osu.Game/Beatmaps/APIBeatmapMetadataSource.cs index e1b01aaac5..9f76aaf02c 100644 --- a/osu.Game/Beatmaps/APIBeatmapMetadataSource.cs +++ b/osu.Game/Beatmaps/APIBeatmapMetadataSource.cs @@ -20,10 +20,13 @@ namespace osu.Game.Beatmaps public bool Available => api.State.Value == APIState.Online; - public OnlineBeatmapMetadata? Lookup(BeatmapInfo beatmapInfo) + public bool TryLookup(BeatmapInfo beatmapInfo, out OnlineBeatmapMetadata? onlineMetadata) { if (!Available) - return null; + { + onlineMetadata = null; + return false; + } Debug.Assert(beatmapInfo.BeatmapSet != null); @@ -37,7 +40,8 @@ namespace osu.Game.Beatmaps if (req.CompletionState == APIRequestCompletionState.Failed) { logForModel(beatmapInfo.BeatmapSet, $@"Online retrieval failed for {beatmapInfo}"); - return null; + onlineMetadata = null; + return true; } var res = req.Response; @@ -46,7 +50,7 @@ namespace osu.Game.Beatmaps { logForModel(beatmapInfo.BeatmapSet, $@"Online retrieval mapped {beatmapInfo} to {res.OnlineBeatmapSetID} / {res.OnlineID}."); - return new OnlineBeatmapMetadata + onlineMetadata = new OnlineBeatmapMetadata { BeatmapID = res.OnlineID, BeatmapSetID = res.OnlineBeatmapSetID, @@ -58,14 +62,18 @@ namespace osu.Game.Beatmaps MD5Hash = res.MD5Hash, LastUpdated = res.LastUpdated }; + return true; } } catch (Exception e) { logForModel(beatmapInfo.BeatmapSet, $@"Online retrieval failed for {beatmapInfo} ({e.Message})"); + onlineMetadata = null; + return false; } - return null; + onlineMetadata = null; + return false; } private void logForModel(BeatmapSetInfo set, string message) => diff --git a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs b/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs index 3e06907c32..b32310990c 100644 --- a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs +++ b/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs @@ -93,18 +93,12 @@ namespace osu.Game.Beatmaps /// private bool tryLookup(BeatmapInfo beatmapInfo, bool preferOnlineFetch, out OnlineBeatmapMetadata? result) { - if (localCachedMetadataSource.Available && (!apiMetadataSource.Available || !preferOnlineFetch)) - { - result = localCachedMetadataSource.Lookup(beatmapInfo); - if (result != null) - return true; - } - - if (apiMetadataSource.Available) - { - result = apiMetadataSource.Lookup(beatmapInfo); + bool useLocalCache = !apiMetadataSource.Available || !preferOnlineFetch; + if (useLocalCache && localCachedMetadataSource.TryLookup(beatmapInfo, out result)) + return true; + + if (apiMetadataSource.TryLookup(beatmapInfo, out result)) return true; - } result = null; return false; diff --git a/osu.Game/Beatmaps/IOnlineBeatmapMetadataSource.cs b/osu.Game/Beatmaps/IOnlineBeatmapMetadataSource.cs index a068e92f95..8230ef40ac 100644 --- a/osu.Game/Beatmaps/IOnlineBeatmapMetadataSource.cs +++ b/osu.Game/Beatmaps/IOnlineBeatmapMetadataSource.cs @@ -18,9 +18,15 @@ namespace osu.Game.Beatmaps /// /// Looks up the online metadata for the supplied . /// + /// The to look up. + /// + /// An instance if the lookup is successful. + /// if a mismatch between the local instance and the looked-up data was detected. + /// The returned value is only valid if the return value of the method is . + /// /// - /// An instance if the lookup is successful, or if the lookup did not return a matching beatmap. + /// Whether the lookup was performed. /// - OnlineBeatmapMetadata? Lookup(BeatmapInfo beatmapInfo); + bool TryLookup(BeatmapInfo beatmapInfo, out OnlineBeatmapMetadata? onlineMetadata); } } diff --git a/osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs b/osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs index 435242aeab..7a179b281f 100644 --- a/osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs +++ b/osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs @@ -108,15 +108,21 @@ namespace osu.Game.Beatmaps // cached database exists on disk. && storage.Exists(cache_database_name); - public OnlineBeatmapMetadata? Lookup(BeatmapInfo beatmapInfo) + public bool TryLookup(BeatmapInfo beatmapInfo, out OnlineBeatmapMetadata? onlineMetadata) { if (!Available) - return null; + { + onlineMetadata = null; + return false; + } if (string.IsNullOrEmpty(beatmapInfo.MD5Hash) && string.IsNullOrEmpty(beatmapInfo.Path) && beatmapInfo.OnlineID <= 0) - return null; + { + onlineMetadata = null; + return false; + } Debug.Assert(beatmapInfo.BeatmapSet != null); @@ -141,7 +147,7 @@ namespace osu.Game.Beatmaps { logForModel(beatmapInfo.BeatmapSet, $@"Cached local retrieval for {beatmapInfo}."); - return new OnlineBeatmapMetadata + onlineMetadata = new OnlineBeatmapMetadata { BeatmapSetID = reader.GetInt32(0), BeatmapID = reader.GetInt32(1), @@ -152,6 +158,7 @@ namespace osu.Game.Beatmaps LastUpdated = reader.GetDateTimeOffset(5), // TODO: DateSubmitted and DateRanked are not provided by local cache. }; + return true; } } } @@ -160,9 +167,12 @@ namespace osu.Game.Beatmaps catch (Exception ex) { logForModel(beatmapInfo.BeatmapSet, $@"Cached local retrieval for {beatmapInfo} failed with {ex}."); + onlineMetadata = null; + return false; } - return null; + onlineMetadata = null; + return false; } private void logForModel(BeatmapSetInfo set, string message) =>