From 2ed3d0bc213bf42b252fdfd4d6c9e922fb48ea5e Mon Sep 17 00:00:00 2001 From: blinkysc <37940565+blinkysc@users.noreply.github.com> Date: Mon, 16 Feb 2026 09:27:23 -0600 Subject: [PATCH] fix(Core/Arena): Fix inverted OnBeforeArenaTeamMemberUpdate hook (#24734) Co-authored-by: blinkysc --- src/server/game/Battlegrounds/Arena.cpp | 4 +- .../Battlegrounds/ArenaHookDefaultsTest.cpp | 129 ++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 src/test/server/game/Battlegrounds/ArenaHookDefaultsTest.cpp diff --git a/src/server/game/Battlegrounds/Arena.cpp b/src/server/game/Battlegrounds/Arena.cpp index cc24d9418..a3dc1432e 100644 --- a/src/server/game/Battlegrounds/Arena.cpp +++ b/src/server/game/Battlegrounds/Arena.cpp @@ -353,13 +353,13 @@ void Arena::EndBattleground(TeamId winnerTeamId) if (GetArenaType() == ARENA_TYPE_5v5 && aliveWinners == 1 && player->IsAlive()) player->CastSpell(player, SPELL_LAST_MAN_STANDING, true); - if (!sScriptMgr->OnBeforeArenaTeamMemberUpdate(winnerArenaTeam, player, true, loserMatchmakerRating, winnerMatchmakerChange)) + if (sScriptMgr->OnBeforeArenaTeamMemberUpdate(winnerArenaTeam, player, true, loserMatchmakerRating, winnerMatchmakerChange)) winnerArenaTeam->MemberWon(player, loserMatchmakerRating, winnerMatchmakerChange); } } else { - if (!sScriptMgr->OnBeforeArenaTeamMemberUpdate(loserArenaTeam, player, false, winnerMatchmakerRating, loserMatchmakerChange)) + if (sScriptMgr->OnBeforeArenaTeamMemberUpdate(loserArenaTeam, player, false, winnerMatchmakerRating, loserMatchmakerChange)) loserArenaTeam->MemberLost(player, winnerMatchmakerRating, loserMatchmakerChange); // Arena lost => reset the win_rated_arena having the "no_lose" condition diff --git a/src/test/server/game/Battlegrounds/ArenaHookDefaultsTest.cpp b/src/test/server/game/Battlegrounds/ArenaHookDefaultsTest.cpp new file mode 100644 index 000000000..e3ceb6299 --- /dev/null +++ b/src/test/server/game/Battlegrounds/ArenaHookDefaultsTest.cpp @@ -0,0 +1,129 @@ +/* + * This file is part of the AzerothCore Project. See AUTHORS file for Copyright information + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see . + */ + +#include "ScriptMgr.h" +#include "ScriptDefines/ArenaScript.h" +#include "ScriptDefines/AllBattlegroundScript.h" +#include "ArenaTeam.h" +#include "ObjectGuid.h" +#include "WorldMock.h" +#include "gtest/gtest.h" + +/** + * Tests that ArenaScript and AllBattlegroundScript hooks return + * safe defaults when no scripts are registered, ensuring core + * game logic (MemberWon/MemberLost, SaveToDB, etc.) is not + * accidentally skipped. + */ +class ArenaHookDefaultsTest : public ::testing::Test +{ +protected: + static void EnsureScriptRegistriesInitialized() + { + static bool initialized = false; + if (!initialized) + { + ScriptRegistry::InitEnabledHooksIfNeeded(ARENAHOOK_END); + ScriptRegistry::InitEnabledHooksIfNeeded(ALLBATTLEGROUNDHOOK_END); + initialized = true; + } + } + + void SetUp() override + { + previousWorld_ = std::move(sWorld); + auto* mock = new ::testing::NiceMock(); + ON_CALL(*mock, getIntConfig(::testing::_)) + .WillByDefault(::testing::Return(0)); + ON_CALL(*mock, getIntConfig(CONFIG_LEGACY_ARENA_START_RATING)) + .WillByDefault(::testing::Return(1500)); + ON_CALL(*mock, getIntConfig(CONFIG_ARENA_START_RATING)) + .WillByDefault(::testing::Return(0)); + sWorld.reset(mock); + + EnsureScriptRegistriesInitialized(); + } + + void TearDown() override + { + sWorld = std::move(previousWorld_); + } + + std::unique_ptr previousWorld_; +}; + +// CanSaveToDB must return true by default so ArenaTeam::SaveToDB +// proceeds to write team and member stats to the database. +TEST_F(ArenaHookDefaultsTest, CanSaveToDBDefaultsTrue) +{ + ArenaTeam team; + EXPECT_TRUE(sScriptMgr->CanSaveToDB(&team)); +} + +// OnBeforeArenaTeamMemberUpdate must return true by default so that +// MemberWon/MemberLost execute. A false return would skip personal +// rating and game count updates for all arena participants. +TEST_F(ArenaHookDefaultsTest, OnBeforeArenaTeamMemberUpdateDefaultsTrue) +{ + ArenaTeam team; + EXPECT_TRUE(sScriptMgr->OnBeforeArenaTeamMemberUpdate(&team, nullptr, true, 1500, 0)); + EXPECT_TRUE(sScriptMgr->OnBeforeArenaTeamMemberUpdate(&team, nullptr, false, 1500, 0)); +} + +// CanSaveArenaStatsForMember must return true by default so that +// character_arena_stats (MMR, MaxMMR) are written to the database. +TEST_F(ArenaHookDefaultsTest, CanSaveArenaStatsForMemberDefaultsTrue) +{ + ArenaTeam team; + EXPECT_TRUE(sScriptMgr->CanSaveArenaStatsForMember(&team, ObjectGuid::Empty)); +} + +// OnBeforeArenaCheckWinConditions must return true by default so +// the normal win condition check proceeds. +TEST_F(ArenaHookDefaultsTest, OnBeforeArenaCheckWinConditionsDefaultsTrue) +{ + EXPECT_TRUE(sScriptMgr->OnBeforeArenaCheckWinConditions(nullptr)); +} + +// CanAddGroupToMatchingPool must return true by default so groups +// are not filtered out of the BG matchmaking pool. +TEST_F(ArenaHookDefaultsTest, CanAddGroupToMatchingPoolDefaultsTrue) +{ + EXPECT_TRUE(sScriptMgr->CanAddGroupToMatchingPool(nullptr, nullptr, 0, nullptr, BattlegroundBracketId(0))); +} + +// Verify the calling convention used in Arena::EndBattleground: +// if (sScriptMgr->OnBeforeArenaTeamMemberUpdate(...)) +// team->MemberWon/MemberLost(...) +// +// The hook returns true when no scripts override it. +// The caller must NOT negate the result, or MemberWon/MemberLost +// will never execute and arena stats will silently stop saving. +TEST_F(ArenaHookDefaultsTest, MemberUpdateCallingConventionAllowsByDefault) +{ + ArenaTeam team; + bool hookResult = sScriptMgr->OnBeforeArenaTeamMemberUpdate( + &team, nullptr, true, 1500, 0); + + // This simulates the condition in Arena::EndBattleground. + // MemberWon must be called when no scripts are registered. + bool memberWonWouldExecute = hookResult; // NOT !hookResult + EXPECT_TRUE(memberWonWouldExecute) + << "MemberWon/MemberLost must execute when no scripts override " + "OnBeforeArenaTeamMemberUpdate. Check Arena.cpp is using " + "if(sScriptMgr->OnBeforeArenaTeamMemberUpdate(...)) without negation."; +}