From ba5bc36f2d5b492ad9c318708fa854e7158aff99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Oct 2023 20:46:38 +0100 Subject: [PATCH 1/7] Add test coverage of correctly-handled mod scenarios --- tests/Models/Multiplayer/ScoreLinkTest.php | 141 +++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 tests/Models/Multiplayer/ScoreLinkTest.php diff --git a/tests/Models/Multiplayer/ScoreLinkTest.php b/tests/Models/Multiplayer/ScoreLinkTest.php new file mode 100644 index 00000000000..06bfedb8d8e --- /dev/null +++ b/tests/Models/Multiplayer/ScoreLinkTest.php @@ -0,0 +1,141 @@ +. Licensed under the GNU Affero General Public License v3.0. +// See the LICENCE file in the repository root for full licence text. + +declare(strict_types=1); + +namespace Tests\Models\Multiplayer; + +use App\Exceptions\InvariantException; +use App\Models\Beatmap; +use App\Models\Multiplayer\PlaylistItem; +use App\Models\Multiplayer\ScoreLink; +use App\Models\User; +use Carbon\Carbon; +use Tests\TestCase; + +class ScoreLinkTest extends TestCase +{ + public function testRequiredModsMissing() + { + $user = User::factory()->create(); + $beatmap = Beatmap::factory()->create(); + $playlistItem = PlaylistItem::factory()->create([ + 'required_mods' => [[ + 'acronym' => 'HD' + ]], + ]); + $scoreLink = ScoreLink::factory()->create([ + 'user_id' => $user, + 'playlist_item_id' => $playlistItem + ]); + + $this->expectException(InvariantException::class); + $this->expectExceptionMessage("This play does not include the mods required."); + $scoreLink->complete([ + 'beatmap_id' => $beatmap->getKey(), + 'ruleset_id' => 0, + 'user_id' => $user->getKey(), + 'ended_at' => json_date(Carbon::now()), + 'mods' => [], + 'statistics' => [ + 'great' => 1 + ] + ]); + } + + public function testRequiredModsPresent() + { + $user = User::factory()->create(); + $beatmap = Beatmap::factory()->create(); + $playlistItem = PlaylistItem::factory()->create([ + 'required_mods' => [[ + 'acronym' => 'HD' + ]], + ]); + $scoreLink = ScoreLink::factory()->create([ + 'user_id' => $user, + 'playlist_item_id' => $playlistItem + ]); + + $this->expectNotToPerformAssertions(); + $scoreLink->complete([ + 'beatmap_id' => $beatmap->getKey(), + 'ruleset_id' => 0, + 'user_id' => $user->getKey(), + 'ended_at' => json_date(Carbon::now()), + 'mods' => [['acronym' => 'HD']], + 'statistics' => [ + 'great' => 1 + ] + ]); + } + + public function testExpectedAllowedMod() + { + $user = User::factory()->create(); + $beatmap = Beatmap::factory()->create(); + $playlistItem = PlaylistItem::factory()->create([ + 'required_mods' => [[ + 'acronym' => 'DT' + ]], + 'allowed_mods' => [[ + 'acronym' => 'HD' + ]], + ]); + $scoreLink = ScoreLink::factory()->create([ + 'user_id' => $user, + 'playlist_item_id' => $playlistItem + ]); + + $this->expectNotToPerformAssertions(); + $scoreLink->complete([ + 'beatmap_id' => $beatmap->getKey(), + 'ruleset_id' => 0, + 'user_id' => $user->getKey(), + 'ended_at' => json_date(Carbon::now()), + 'mods' => [ + ['acronym' => 'DT'], + ['acronym' => 'HD'] + ], + 'statistics' => [ + 'great' => 1 + ] + ]); + } + + public function testUnexpectedAllowedMod() + { + $user = User::factory()->create(); + $beatmap = Beatmap::factory()->create(); + $playlistItem = PlaylistItem::factory()->create([ + 'required_mods' => [[ + 'acronym' => 'DT' + ]], + 'allowed_mods' => [[ + 'acronym' => 'HR' + ]], + ]); + $scoreLink = ScoreLink::factory()->create([ + 'user_id' => $user, + 'playlist_item_id' => $playlistItem + ]); + + $this->expectException(InvariantException::class); + $this->expectExceptionMessage("This play includes mods that are not allowed."); + $scoreLink->complete([ + 'beatmap_id' => $beatmap->getKey(), + 'ruleset_id' => 0, + 'user_id' => $user->getKey(), + 'ended_at' => json_date(Carbon::now()), + 'mods' => [ + ['acronym' => 'DT'], + ['acronym' => 'HD'] + ], + 'statistics' => [ + 'great' => 1 + ] + ]); + } +} From c410b6f6be1c9cd8bdd7809c0062d422fc3f6238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Oct 2023 20:47:45 +0100 Subject: [PATCH 2/7] Add failing test coverage for extraneous mods when no allowed mods specified --- tests/Models/Multiplayer/ScoreLinkTest.php | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/Models/Multiplayer/ScoreLinkTest.php b/tests/Models/Multiplayer/ScoreLinkTest.php index 06bfedb8d8e..cfc8d97be10 100644 --- a/tests/Models/Multiplayer/ScoreLinkTest.php +++ b/tests/Models/Multiplayer/ScoreLinkTest.php @@ -138,4 +138,28 @@ public function testUnexpectedAllowedMod() ] ]); } + + public function testUnexpectedModWhenNoModsAreAllowed() + { + $user = User::factory()->create(); + $beatmap = Beatmap::factory()->create(); + $playlistItem = PlaylistItem::factory()->create(); // no required or allowed mods. + $scoreLink = ScoreLink::factory()->create([ + 'user_id' => $user, + 'playlist_item_id' => $playlistItem + ]); + + $this->expectException(InvariantException::class); + $this->expectExceptionMessage("This play includes mods that are not allowed."); + $scoreLink->complete([ + 'beatmap_id' => $beatmap->getKey(), + 'ruleset_id' => 0, + 'user_id' => $user->getKey(), + 'ended_at' => json_date(Carbon::now()), + 'mods' => [['acronym' => 'HD']], + 'statistics' => [ + 'great' => 1 + ] + ]); + } } From e23ca6bd36c75d8253beaa01b760fcf23a812fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Oct 2023 20:51:42 +0100 Subject: [PATCH 3/7] Fix multiplayer score mods not being validated properly when playlist item has no allowed mods The `allowed_mods` list being empty is not supposed to mean that any mod is allowed. All allowed mods must (and will) be explicitly listed in `allowed_mods`; if the list is empty, then it means that the score's mods must be exactly set-equal to `required_mods`. --- app/Models/Multiplayer/ScoreLink.php | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/app/Models/Multiplayer/ScoreLink.php b/app/Models/Multiplayer/ScoreLink.php index 1ebe58852ee..21e0157c6fb 100644 --- a/app/Models/Multiplayer/ScoreLink.php +++ b/app/Models/Multiplayer/ScoreLink.php @@ -81,16 +81,14 @@ public function complete(array $params): void } } - if (!empty($this->playlistItem->allowed_mods)) { - $missingMods = array_diff( - array_column($mods, 'acronym'), - array_column($this->playlistItem->required_mods, 'acronym'), - array_column($this->playlistItem->allowed_mods, 'acronym') - ); - - if (!empty($missingMods)) { - throw new InvariantException('This play includes mods that are not allowed.'); - } + $missingMods = array_diff( + array_column($mods, 'acronym'), + array_column($this->playlistItem->required_mods, 'acronym'), + array_column($this->playlistItem->allowed_mods, 'acronym') + ); + + if (!empty($missingMods)) { + throw new InvariantException('This play includes mods that are not allowed.'); } $this->score()->associate($score); From 2b8e9ec709a14d5f1950b9f8c42727ab4aef127c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Oct 2023 20:54:47 +0100 Subject: [PATCH 4/7] Rename variable --- app/Models/Multiplayer/ScoreLink.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Models/Multiplayer/ScoreLink.php b/app/Models/Multiplayer/ScoreLink.php index 21e0157c6fb..3ca353a62ed 100644 --- a/app/Models/Multiplayer/ScoreLink.php +++ b/app/Models/Multiplayer/ScoreLink.php @@ -81,13 +81,13 @@ public function complete(array $params): void } } - $missingMods = array_diff( + $disallowedMods = array_diff( array_column($mods, 'acronym'), array_column($this->playlistItem->required_mods, 'acronym'), array_column($this->playlistItem->allowed_mods, 'acronym') ); - if (!empty($missingMods)) { + if (!empty($disallowedMods)) { throw new InvariantException('This play includes mods that are not allowed.'); } From 4df11268fb0599e95aed864a4ec9a0c8ea66c660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Oct 2023 22:15:21 +0100 Subject: [PATCH 5/7] Fix lint issues --- tests/Models/Multiplayer/ScoreLinkTest.php | 52 +++++++++++----------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/Models/Multiplayer/ScoreLinkTest.php b/tests/Models/Multiplayer/ScoreLinkTest.php index cfc8d97be10..b9c5b8f841b 100644 --- a/tests/Models/Multiplayer/ScoreLinkTest.php +++ b/tests/Models/Multiplayer/ScoreLinkTest.php @@ -23,16 +23,16 @@ public function testRequiredModsMissing() $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ - 'acronym' => 'HD' + 'acronym' => 'HD', ]], ]); $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, - 'playlist_item_id' => $playlistItem + 'playlist_item_id' => $playlistItem, ]); $this->expectException(InvariantException::class); - $this->expectExceptionMessage("This play does not include the mods required."); + $this->expectExceptionMessage('This play does not include the mods required.'); $scoreLink->complete([ 'beatmap_id' => $beatmap->getKey(), 'ruleset_id' => 0, @@ -40,8 +40,8 @@ public function testRequiredModsMissing() 'ended_at' => json_date(Carbon::now()), 'mods' => [], 'statistics' => [ - 'great' => 1 - ] + 'great' => 1, + ], ]); } @@ -51,12 +51,12 @@ public function testRequiredModsPresent() $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ - 'acronym' => 'HD' + 'acronym' => 'HD', ]], ]); $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, - 'playlist_item_id' => $playlistItem + 'playlist_item_id' => $playlistItem, ]); $this->expectNotToPerformAssertions(); @@ -67,8 +67,8 @@ public function testRequiredModsPresent() 'ended_at' => json_date(Carbon::now()), 'mods' => [['acronym' => 'HD']], 'statistics' => [ - 'great' => 1 - ] + 'great' => 1, + ], ]); } @@ -78,15 +78,15 @@ public function testExpectedAllowedMod() $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ - 'acronym' => 'DT' + 'acronym' => 'DT', ]], 'allowed_mods' => [[ - 'acronym' => 'HD' + 'acronym' => 'HD', ]], ]); $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, - 'playlist_item_id' => $playlistItem + 'playlist_item_id' => $playlistItem, ]); $this->expectNotToPerformAssertions(); @@ -97,11 +97,11 @@ public function testExpectedAllowedMod() 'ended_at' => json_date(Carbon::now()), 'mods' => [ ['acronym' => 'DT'], - ['acronym' => 'HD'] + ['acronym' => 'HD'], ], 'statistics' => [ - 'great' => 1 - ] + 'great' => 1, + ], ]); } @@ -111,19 +111,19 @@ public function testUnexpectedAllowedMod() $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ - 'acronym' => 'DT' + 'acronym' => 'DT', ]], 'allowed_mods' => [[ - 'acronym' => 'HR' + 'acronym' => 'HR', ]], ]); $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, - 'playlist_item_id' => $playlistItem + 'playlist_item_id' => $playlistItem, ]); $this->expectException(InvariantException::class); - $this->expectExceptionMessage("This play includes mods that are not allowed."); + $this->expectExceptionMessage('This play includes mods that are not allowed.'); $scoreLink->complete([ 'beatmap_id' => $beatmap->getKey(), 'ruleset_id' => 0, @@ -131,11 +131,11 @@ public function testUnexpectedAllowedMod() 'ended_at' => json_date(Carbon::now()), 'mods' => [ ['acronym' => 'DT'], - ['acronym' => 'HD'] + ['acronym' => 'HD'], ], 'statistics' => [ - 'great' => 1 - ] + 'great' => 1, + ], ]); } @@ -146,11 +146,11 @@ public function testUnexpectedModWhenNoModsAreAllowed() $playlistItem = PlaylistItem::factory()->create(); // no required or allowed mods. $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, - 'playlist_item_id' => $playlistItem + 'playlist_item_id' => $playlistItem, ]); $this->expectException(InvariantException::class); - $this->expectExceptionMessage("This play includes mods that are not allowed."); + $this->expectExceptionMessage('This play includes mods that are not allowed.'); $scoreLink->complete([ 'beatmap_id' => $beatmap->getKey(), 'ruleset_id' => 0, @@ -158,8 +158,8 @@ public function testUnexpectedModWhenNoModsAreAllowed() 'ended_at' => json_date(Carbon::now()), 'mods' => [['acronym' => 'HD']], 'statistics' => [ - 'great' => 1 - ] + 'great' => 1, + ], ]); } } From e04446f9560f55efd1d4d77a9094405aef6baefa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 1 Nov 2023 09:04:32 +0100 Subject: [PATCH 6/7] Fix double-creating beatmaps in test --- tests/Models/Multiplayer/ScoreLinkTest.php | 26 +++++++++------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tests/Models/Multiplayer/ScoreLinkTest.php b/tests/Models/Multiplayer/ScoreLinkTest.php index b9c5b8f841b..f088c618517 100644 --- a/tests/Models/Multiplayer/ScoreLinkTest.php +++ b/tests/Models/Multiplayer/ScoreLinkTest.php @@ -8,7 +8,6 @@ namespace Tests\Models\Multiplayer; use App\Exceptions\InvariantException; -use App\Models\Beatmap; use App\Models\Multiplayer\PlaylistItem; use App\Models\Multiplayer\ScoreLink; use App\Models\User; @@ -20,7 +19,6 @@ class ScoreLinkTest extends TestCase public function testRequiredModsMissing() { $user = User::factory()->create(); - $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ 'acronym' => 'HD', @@ -34,8 +32,8 @@ public function testRequiredModsMissing() $this->expectException(InvariantException::class); $this->expectExceptionMessage('This play does not include the mods required.'); $scoreLink->complete([ - 'beatmap_id' => $beatmap->getKey(), - 'ruleset_id' => 0, + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), 'ended_at' => json_date(Carbon::now()), 'mods' => [], @@ -48,7 +46,6 @@ public function testRequiredModsMissing() public function testRequiredModsPresent() { $user = User::factory()->create(); - $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ 'acronym' => 'HD', @@ -61,8 +58,8 @@ public function testRequiredModsPresent() $this->expectNotToPerformAssertions(); $scoreLink->complete([ - 'beatmap_id' => $beatmap->getKey(), - 'ruleset_id' => 0, + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), 'ended_at' => json_date(Carbon::now()), 'mods' => [['acronym' => 'HD']], @@ -75,7 +72,6 @@ public function testRequiredModsPresent() public function testExpectedAllowedMod() { $user = User::factory()->create(); - $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ 'acronym' => 'DT', @@ -91,8 +87,8 @@ public function testExpectedAllowedMod() $this->expectNotToPerformAssertions(); $scoreLink->complete([ - 'beatmap_id' => $beatmap->getKey(), - 'ruleset_id' => 0, + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), 'ended_at' => json_date(Carbon::now()), 'mods' => [ @@ -108,7 +104,6 @@ public function testExpectedAllowedMod() public function testUnexpectedAllowedMod() { $user = User::factory()->create(); - $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create([ 'required_mods' => [[ 'acronym' => 'DT', @@ -125,8 +120,8 @@ public function testUnexpectedAllowedMod() $this->expectException(InvariantException::class); $this->expectExceptionMessage('This play includes mods that are not allowed.'); $scoreLink->complete([ - 'beatmap_id' => $beatmap->getKey(), - 'ruleset_id' => 0, + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), 'ended_at' => json_date(Carbon::now()), 'mods' => [ @@ -142,7 +137,6 @@ public function testUnexpectedAllowedMod() public function testUnexpectedModWhenNoModsAreAllowed() { $user = User::factory()->create(); - $beatmap = Beatmap::factory()->create(); $playlistItem = PlaylistItem::factory()->create(); // no required or allowed mods. $scoreLink = ScoreLink::factory()->create([ 'user_id' => $user, @@ -152,8 +146,8 @@ public function testUnexpectedModWhenNoModsAreAllowed() $this->expectException(InvariantException::class); $this->expectExceptionMessage('This play includes mods that are not allowed.'); $scoreLink->complete([ - 'beatmap_id' => $beatmap->getKey(), - 'ruleset_id' => 0, + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), 'ended_at' => json_date(Carbon::now()), 'mods' => [['acronym' => 'HD']], From 493efc0af762530a93f5f99411d52a5afd1fd375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 1 Nov 2023 09:15:23 +0100 Subject: [PATCH 7/7] Fix tests asserting on already-populated-and-persisted score links --- tests/Models/Multiplayer/ScoreLinkTest.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/Models/Multiplayer/ScoreLinkTest.php b/tests/Models/Multiplayer/ScoreLinkTest.php index f088c618517..363cfc2462e 100644 --- a/tests/Models/Multiplayer/ScoreLinkTest.php +++ b/tests/Models/Multiplayer/ScoreLinkTest.php @@ -24,7 +24,8 @@ public function testRequiredModsMissing() 'acronym' => 'HD', ]], ]); - $scoreLink = ScoreLink::factory()->create([ + $scoreLink = ScoreLink::factory()->make([ + 'score_id' => null, 'user_id' => $user, 'playlist_item_id' => $playlistItem, ]); @@ -51,7 +52,8 @@ public function testRequiredModsPresent() 'acronym' => 'HD', ]], ]); - $scoreLink = ScoreLink::factory()->create([ + $scoreLink = ScoreLink::factory()->make([ + 'score_id' => null, 'user_id' => $user, 'playlist_item_id' => $playlistItem, ]); @@ -80,7 +82,8 @@ public function testExpectedAllowedMod() 'acronym' => 'HD', ]], ]); - $scoreLink = ScoreLink::factory()->create([ + $scoreLink = ScoreLink::factory()->make([ + 'score_id' => null, 'user_id' => $user, 'playlist_item_id' => $playlistItem, ]); @@ -112,7 +115,8 @@ public function testUnexpectedAllowedMod() 'acronym' => 'HR', ]], ]); - $scoreLink = ScoreLink::factory()->create([ + $scoreLink = ScoreLink::factory()->make([ + 'score_id' => null, 'user_id' => $user, 'playlist_item_id' => $playlistItem, ]); @@ -138,7 +142,8 @@ public function testUnexpectedModWhenNoModsAreAllowed() { $user = User::factory()->create(); $playlistItem = PlaylistItem::factory()->create(); // no required or allowed mods. - $scoreLink = ScoreLink::factory()->create([ + $scoreLink = ScoreLink::factory()->make([ + 'score_id' => null, 'user_id' => $user, 'playlist_item_id' => $playlistItem, ]);