Skip to content

Add a unit test suite (doctest, MSVC Win32) and run it in CI - #98

Open
errolgr wants to merge 8 commits into
Project-Diablo-2:mainfrom
errolgr:tests/unit-suite
Open

errolgr wants to merge 8 commits into
Project-Diablo-2:mainfrom
errolgr:tests/unit-suite

Conversation

@errolgr

@errolgr errolgr commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Summary

This adds tests/BH.Tests.vcxproj, a console program that compiles BH's own sources unmodified and runs 505 doctest test cases (6,078 assertions) against them. CI builds and runs it on every pull request, next to the existing BH.dll build, and the run fails if any test fails. No production code changed.

The tests cover the loot filter end to end (conditions, formulas, actions, rule lists, aliases, filter levels, caches), the config layer (filter file parsing, BH.json load/save/backup recovery), JSONObject and the Mustache stash export templates, the game list filter, and a set of modules and UI controls (stats panel tables, automap info, party, chat colour, map notifications, Bnet, Keyhook/Checkhook/Combohook).

Along the way the suite found 21 real bugs. Each one has a test asserting the correct behaviour, marked should_fail with a // BUG: comment, so CI stays green today and turns red once the bug is fixed (then the marker gets removed).

Problem

BH has no automated tests. The loot filter alone is around 5,600 lines of parsing and evaluation that every PD2 player depends on, and the only way to check a change today is to load a filter in game and look at items. Regressions in keyword parsing, operator handling or name output are easy to ship and hard to spot.

Changes

Area What
ThirdParty/doctest/ doctest 2.4.12 single header, MIT licence file kept
tests/BH.Tests.vcxproj Win32 console exe, same toolset/charset/include layout as BH.vcxproj. Compiles the BH sources under test directly. Not added to BH.sln
tests/fakes/EnginePtrs.cpp Defines the D2Ptrs.h pointers like BH.cpp does (_DEFINE_PTRS). Patch::GetDllOffset is replaced: engine functions the tests need resolve to fakes (the ROUTE macro type-checks each fake against the real signature), everything else resolves to zeroed memory. D2Version is fixed to 1.13c
tests/fakes/FakeEngine.* Small model of the game state those functions read: units and stat lists, item txt records, locale strings, player, difficulty, area, prices per transaction type, level requirement per class, drawn text, sent packets
tests/fakes/BHGlobals.cpp Globals owned by BH.cpp / Item.cpp / Module.cpp, which are not compiled (they install hooks)
tests/support/LootFilter.* TestItem, Matches, LoadFilter (real Config::Parse + ItemDisplay::InitializeItemRules on a temp file), NameOf, DescriptionOf
tests/*Tests.cpp 22 suites, one file per area
.github/workflows/build.yml New tests job: build, run (fails the run on any failure), write and upload a JUnit report. The existing BH.dll job is untouched
README.md How to run the tests

All game and BH state is reset before every test case, so tests are independent and also pass in random order (--order-by=rand).

Why a separate .vcxproj and not CMake

Maintainers build with Visual Studio and MSBuild, and CI already uses msbuild. A plain .vcxproj next to the code needs nothing new to install and uses the same compiler and settings as BH.dll, so a test that passes is testing the same compilation. The CMakeLists.txt files in the repo are out of date (they list files that no longer exist), so a CMake target would mean maintaining a second build description. Keeping the project out of BH.sln means building BH in Visual Studio is exactly as before; anyone who wants the tests opens tests\BH.Tests.vcxproj or runs one msbuild line.

Test areas

Suite Test cases Assertions Known-bug tests
ItemConditions (loot filter condition keywords, grammar) 51 2128 2
ItemStatConditions (STAT/SK/RES/ED/REQ/UP/damage/sums) 43 1030 3
Formula (expression language) 33 817 0
Formula loot filter (Formula[], $f() islands, variables) 25 232 2
LootFilterActions (colours, notify/map keywords, replacements, trimming) 49 222 2
LootFilterRules (rule order, %CONTINUE%, aliases, filter levels, caches) 45 191 0
Config (filter file parsing, BH.json load/save/backup) 35 245 5
Common (string/key helpers, PrintText) 21 181 1
JSONObject 19 154 1
Mustache (stash export templates incl. BH extensions and defaults) 33 198 1
GameFilter (game list filter string, list building) 31 193 0
StatsDisplay (breakpoint tables, panel) 22 146 2
D2Helpers 12 48 0
ScreenInfo (automap info tokens, monster level) 16 32 0
Party (auto party, corpse loot packets) 18 52 1
MapNotify (drop notifications) 13 50 0
ChatColor (whisper colours) 7 25 1
Bnet (next game name, remembered fields) 9 22 0
Keyhook / Checkhook / Combohook 17 101 0
AsyncDrawBuffer 6 11 0
Total 505 6,078 21

Expected values come from Diablo II / PD2 rules and data (the PD2 Item Filtering wiki, breakpoint tables, Windows virtual-key codes, the Mustache spec, hand calculation), never from running BH and copying its output.

Bugs found (tests marked should_fail)

# Where Bug
1 Common GetKeyCode VK_FORWARDSLASH is 0xBD (same as VK_MINUS) and VK_TILDE is 0xBF (the / key). '/' should be VK_OEM_2, '' VK_OEM_3 (0xC0), so these hotkeys fire on the wrong key and '' can't be bound
2 Config Parse A filter line without ':' becomes a rule whose output is the whole line; the wiki says it should not be a rule
3-6 Config GetToggle/GetKey/GetArray/GetAssoc Unlike GetInt/GetBool/GetString they don't catch nlohmann::type_error, so a wrongly typed BH.json entry throws out of LoadConfig
7 Formula A mixed-case reference (Formulastrong) is dropped from the rule, so the rule matches items the formula rejects (wiki: only the leading F must be upper case)
8 Formula output Negative values that round to zero (e.g. $f(-STAT5) without the stat) render as -0
9 Conditions MAPTIER Non-map items get tier -1, so MAPTIER<N matches every non-map item
10 Conditions A stray ) silently drops the rest of the rule (ETH ) UNI acts as ETH)
11 Conditions MULTI A 10-digit id makes std::stoi throw out of InitializeItemRules, aborting the whole filter load
12 Conditions BASE* dmg Ethereal base damage is (d + d/2) * 5/6, so odd values are low (1 gives 0, 5 gives 5) instead of x1.25
13 Conditions + sums ~ ranges on sums ignore the upper bound (FRES+CRES~50-100 never matches)
14 Actions legacy %MAP% Only the first occurrence of a colour keyword is found, so a colour repeated before %MAP% picks the wrong minimap colour
15 Actions %TIER-n% Only one digit is parsed; the wiki documents tiers 0-12, so %TIER-10%..%TIER-12% are ignored and shown as text
16 JSONObject Json_Escape Control characters other than \b\f\n\r\t are copied raw, producing invalid JSON
17 Mustache / JSONObject An integer 0 in the outermost context renders as 0.000000
18 StatsDisplay The Act 4 mercenary uses the 7-frame wolf-form FHR table instead of a 13-frame table
19 StatsDisplay On 800x600, two extra stats make the panel taller than the screen; SetYSize rejects the height instead of clamping, so the open panel no longer covers its rows
20 ChatColor Whisper colour is parsed with an unguarded std::stoi; a non-numeric colour throws out of the chat packet handler
21 Party The "party id but not in a party" guard tests flags & PARTY_NOT_IN_PARTY, which is 0, so it never fires and the player leaves their party in that window

Also noticed but not tested (undefined behaviour or no deterministic way to test without changing code): commaprint returns a pointer to a stack buffer; string_format("") writes past a zero-length buffer; a self-referencing Alias loops forever in InitializeItemRules; ScreenInfo's exp/s rate wraps to billions after a death (unsigned subtraction); Task's canceled_ is never initialised.

Production code seams

None. Everything is done from the test side: the engine pointers are defined by the test project, and the few BH globals and module classes that live in files the tests don't compile (BH.cpp, Item.cpp, Module.cpp, ModuleManager.cpp) get small stand-ins in tests/fakes/BHGlobals.cpp.

Testing

CI proof

Run Branch Result Link
Suite tests/unit-suite green, test cases: 505 | 505 passed | 0 failed, BH.dll job unchanged https://github.com/errolgr/BH/actions/runs/36823705995
Gate check (one deliberately broken test, throwaway branch, since deleted) tests/gate-check red: Run tests step failed, test cases: 506 | 505 passed | 1 failed https://github.com/errolgr/BH/actions/runs/36823729966

Coverage

Measured locally with clang source-based coverage (-fprofile-instr-generate -fcoverage-mapping, llvm-cov) on a Win32 build of the same test project, for the BH files the tests target. Tooling is not part of this PR.

File Lines Line % Functions Function %
Mustache.cpp 332 / 332 100.0% 26 / 26 100.0%
JSONObject.cpp 501 / 539 92.9% 68 / 73 93.2%
Modules/Gamefilter/ParsedFilterString.cpp 73 / 79 92.4% 8 / 9 88.9%
Config.cpp 363 / 396 91.7% 13 / 14 92.9%
RuleLookupCache.h 26 / 29 89.7% 2 / 3 66.7%
AsyncDrawBuffer.cpp 79 / 92 85.9% 18 / 20 90.0%
Modules/ChatColor/ChatColor.cpp 40 / 47 85.1% 4 / 7 57.1%
Modules/Party/Party.cpp 109 / 132 82.6% 3 / 7 42.9%
Drawing/Advanced/Keyhook/Keyhook.cpp 60 / 78 76.9% 6 / 7 85.7%
Modules/Item/ItemDisplay.cpp 2635 / 3620 72.8% 200 / 371 53.9%
Formula.h 355 / 562 63.2% 21 / 21 100.0%
Drawing/Advanced/Checkhook/Checkhook.cpp 61 / 104 58.7% 9 / 15 60.0%
Modules/ScreenInfo/ScreenInfo.cpp 154 / 266 57.9% 5 / 13 38.5%
Drawing/Advanced/Combohook/Combohook.cpp 31 / 55 56.4% 2 / 4 50.0%
Modules/Bnet/Bnet.cpp 78 / 141 55.3% 4 / 13 30.8%
Common.cpp 144 / 284 50.7% 21 / 34 61.8%
Modules/Gamefilter/Gamefilter.cpp 203 / 418 48.6% 3 / 17 17.6%
Modules/MapNotify/MapNotify.cpp 61 / 212 28.8% 3 / 14 21.4%
D2Helpers.cpp 89 / 443 20.1% 8 / 28 28.6%
Drawing/Stats/StatsDisplay.cpp 272 / 1834 14.8% 12 / 19 63.2%
All files above 5666 / 9663 58.6% 436 / 715 61.0%

The low numbers are mostly drawing code that needs the real renderer (StatsDisplay's OnDraw is ~1,500 lines), helpers nothing in BH calls (left untested on purpose, e.g. the geometry helpers in Common.cpp), and in ItemDisplay.cpp the formula variable table, where most of the ~140 variables are not exercised one by one yet (a good follow-up: the wiki says each variable mirrors a condition keyword, which makes a table-driven check easy).

Running locally

msbuild tests\BH.Tests.vcxproj /p:Configuration=Release /p:Platform=Win32
tests\bin\Release\BH.Tests.exe

-ts=LootFilterRules runs one suite, -tc="*TIER*" matches test names, --order-by=rand shuffles.

Single-header test framework (MIT, licence kept in ThirdParty/doctest).
tests/BH.Tests.vcxproj builds a Win32 console exe that compiles BH sources unmodified, plus:
- fakes/EnginePtrs.cpp: defines the D2Ptrs.h engine pointers like BH.cpp does, with
  Patch::GetDllOffset routing the engine functions tests need to fakes and everything else
  to zeroed memory, and D2Version fixed to 1.13c.
- fakes/FakeEngine: a small model of the game state those functions read.
- fakes/BHGlobals.cpp: globals owned by BH.cpp/Item.cpp/Module.cpp, which are not compiled.
- support/LootFilter: helpers to build items and load a filter file through Config::Parse and
  ItemDisplay::InitializeItemRules.
Game and BH state are reset before every test case. The project is not part of BH.sln.
A second job in build.yml builds tests/BH.Tests.vcxproj (MSVC Release|Win32) and runs it; any
failing test fails the run. The doctest summary is in the log and a JUnit report is attached
as an artifact. README explains how to run the tests locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant