Refactor/catch2 tests

.dockerignore

@@ -1,7 +1,6 @@
 # Local build, docs and test output: large, stale and never read by the image build
 build/
 book/
-.venv/
 test/data/
 
 # .git is deliberately not ignored. The root CMakeLists runs git rev-parse to

.github/ISSUE_TEMPLATE/bug_report.yml

@@ -24,7 +24,6 @@ body:
         - Linux
         - Windows
         - WSL (Windows Subsystem for Linux)
-        - macOS
         - Other
     validations:
       required: true

.github/dependabot.yml

@@ -9,14 +9,6 @@ updates:
     assignees:
       - "thomaslaurenson"
 
-  - package-ecosystem: pip
-    directory: /test
-    schedule:
-      interval: weekly
-    open-pull-requests-limit: 0
-    assignees:
-      - "thomaslaurenson"
-
   - package-ecosystem: docker
     directory: /
     schedule:

.github/workflows/build.yml

@@ -92,11 +92,13 @@ jobs:
           submodules: true
 
       # Raw cmake rather than make: the Makefile needs a POSIX shell, and the
-      # shipped build gets its own tree rather than the everyday build/dev
+      # shipped build gets its own tree rather than the everyday build/dev.
+      # Testing is off because this job ships the binary; test.yml builds and
+      # runs the tests on Windows in a tree of its own.
       - name: Build
         shell: bash
         run: |
-          cmake -B build/release -G "Visual Studio 17 2022" -A x64 -DMPQCLI_BUILD_APP=ON
+          cmake -B build/release -G "Visual Studio 17 2022" -A x64 -DMPQCLI_BUILD_APP=ON -DMPQCLI_BUILD_TESTING=OFF
           cmake --build build/release --config Release
           mv build/release/bin/mpqcli.exe ./mpqcli-windows-x86_64.exe
 

.github/workflows/test.yml

@@ -54,45 +54,39 @@ jobs:
       - run: make configure CMAKE_ARGS="-DCMAKE_CXX_COMPILER=${{ matrix.cxx }} -DMPQCLI_WERROR=ON"
       - run: make build
 
-      - name: Setup python
-        uses: actions/setup-python@v6
-        with:
-          python-version: 3.11
-
-      - name: Install dependencies
-        run: |
-          python -m pip install --upgrade pip
-          pip install -r test/requirements.txt
-
-      - name: Run tests
-        run: python3 -m pytest test
+      # Separate steps, so a failure names the layer that broke
+      - run: make test
+      - run: make test_functional
 
   test_windows:
+    name: test_windows (${{ matrix.build_type }})
+    strategy:
+      fail-fast: false
+      matrix:
+        # Debug as well as Release: MSVC's Debug runtime adds checked iterators
+        # and CRT assertions that Release compiles out
+        build_type: [Debug, Release]
+
     runs-on: windows-2022
     steps:
       - name: Check out repository code
         uses: actions/checkout@v6
         with:
           submodules: true
 
-      # Raw cmake: the Makefile needs a POSIX shell. MPQCLI_WERROR stays off here
-      # until the tree is known to compile clean under /W4, which nothing has yet
-      # checked; it is on in the Linux jobs.
+      # Raw cmake: the Makefile needs a POSIX shell. MPQCLI_WERROR holds MSVC to
+      # the same bar as the Linux jobs: /W4 /permissive- with warnings as errors.
       - name: Configure and build
         shell: bash
         run: |
-          cmake -B build/dev -G "Visual Studio 17 2022" -A x64 -DMPQCLI_BUILD_APP=ON
-          cmake --build build/dev --config Release --parallel
-
-      - name: Setup python
-        uses: actions/setup-python@v6
-        with:
-          python-version: 3.11
+          cmake -B build/dev -G "Visual Studio 17 2022" -A x64 -DMPQCLI_BUILD_APP=ON -DMPQCLI_WERROR=ON
+          cmake --build build/dev --config ${{ matrix.build_type }} --parallel
 
-      - name: Install dependencies
-        run: |
-          python -m pip install --upgrade pip
-          pip install -r test/requirements.txt
+      # Separate steps, so a failure names the layer that broke
+      - name: Run unit tests
+        shell: bash
+        run: ctest --test-dir build/dev --output-on-failure -C ${{ matrix.build_type }} -L unit
 
-      - name: Run tests
-        run: python -m pytest test
+      - name: Run functional tests
+        shell: bash
+        run: ctest --test-dir build/dev --output-on-failure -C ${{ matrix.build_type }} -L functional

.gitmodules

@@ -7,3 +7,9 @@
 [submodule "extern/hash-library"]
 	path = extern/hash-library
 	url = https://github.com/stbrumme/hash-library.git
+[submodule "extern/Catch2"]
+	path = extern/Catch2
+	url = https://github.com/catchorg/Catch2.git
+[submodule "extern/subprocess.h"]
+	path = extern/subprocess.h
+	url = https://github.com/sheredom/subprocess.h.git

CMakeLists.txt

@@ -6,6 +6,13 @@ project(mpqcli VERSION 0.13.0)
 # carry the project prefix; a bare BUILD_STATIC claims a name anyone might want.
 option(MPQCLI_BUILD_APP "Build the mpqcli binary" ON)
 option(MPQCLI_BUILD_STATIC "Build static binary" OFF)
+# On only when mpqcli is the top-level project, so a consumer that adds it with
+# add_subdirectory does not get its tests in their own CTest run
+option(MPQCLI_BUILD_TESTING "Build mpqcli tests" ${PROJECT_IS_TOP_LEVEL})
+# The real-world archives a few functional tests read. They are never committed;
+# a test whose archive is absent is skipped rather than failed.
+set(MPQCLI_INTEGRATION_DATA "${PROJECT_SOURCE_DIR}/test/data/integration" CACHE PATH
+    "Directory of real-world archives for the tests that need them")
 # Off by default, so a new compiler release adding a warning cannot stop a
 # developer working. The Linux CI test builds turn it on, and check_lint holds
 # the same bar: clang-tidy replays these flags and .clang-tidy treats every
@@ -72,6 +79,11 @@ set(STORM_USE_BUNDLED_LIBRARIES ON CACHE BOOL "" FORCE)
 
 add_subdirectory(extern/StormLib)
 mark_system(storm)
+# GCC 15 defaults to C23, which drops the K&R function definitions in the bundled
+# zlib. MSVC is left on its own default, which is not C23, so Windows is unchanged.
+if(NOT MSVC)
+    set_target_properties(storm PROPERTIES C_STANDARD 11)
+endif()
 
 # Only app/ parses arguments, so the core never sees CLI11
 if(MPQCLI_BUILD_APP)
@@ -107,10 +119,50 @@ add_library(mpqcli_hash_library STATIC
 target_include_directories(mpqcli_hash_library SYSTEM INTERFACE "${PROJECT_SOURCE_DIR}/extern")
 add_library(mpqcli::hash_library ALIAS mpqcli_hash_library)
 
+# Catch2 runs every test layer, and subprocess.h lets the functional layer spawn
+# the binary
+if(MPQCLI_BUILD_TESTING)
+    if (NOT EXISTS "${PROJECT_SOURCE_DIR}/extern/Catch2/CMakeLists.txt")
+        message(FATAL_ERROR
+"Missing dependency: Catch2
+mpqcli tests require the Catch2 testing framework.
+It is provided as a submodule of this repository.
+Did you forget to execute the following commands?
+   git submodule init
+   git submodule update")
+    endif()
+
+    if (NOT EXISTS "${PROJECT_SOURCE_DIR}/extern/subprocess.h/subprocess.h")
+        message(FATAL_ERROR
+"Missing dependency: subprocess.h
+mpqcli functional tests require the subprocess.h library to spawn the binary.
+It is provided as a submodule of this repository.
+Did you forget to execute the following commands?
+   git submodule init
+   git submodule update")
+    endif()
+
+    # enable_testing() rather than include(CTest): include(CTest) also declares a
+    # global BUILD_TESTING cache variable, which switches on the self-tests of any
+    # submodule that defers to it. It must run before the add_subdirectory that
+    # registers tests, or CTest finds none.
+    enable_testing()
+
+    add_subdirectory(extern/Catch2)
+    mark_system(Catch2)
+
+    list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/extern/Catch2/extras")
+    include(Catch)
+endif()
+
 # Unconditional: building the core without the app is what proves nothing in
 # src/ has picked up a dependency on app/.
 add_subdirectory(src)
 
 if(MPQCLI_BUILD_APP)
     add_subdirectory(app)
 endif()
+
+if(MPQCLI_BUILD_TESTING)
+    add_subdirectory(test)
+endif()

CONTRIBUTING.md

@@ -32,15 +32,17 @@ Run `make help` to list all available targets. Common ones:
 |---|---|
 | `make configure` | Configure the cmake build (uses the default compiler) |
 | `make build` | Build the project into build/dev |
-| `make test_create_venv` | Create Python venv and install test dependencies (first-time only) |
-| `make test_mpqcli` | Run the pytest test suite |
+| `make test` | Run the unit tests |
+| `make test_functional` | Run the functional tests against the built binary |
+| `make test_all` | Run every test layer in build/dev |
+| `make fetch_integration_data` | Download the real archives a few functional tests read (needs filenget) |
 | `make check_all` | Run every static check (clang-format, clang-tidy, completion scripts) |
 | `make check_format` | Check formatting only (dry run) |
 | `make format` | Auto-fix formatting in-place |
 | `make configure_lint` | Configure build/lint with clang++ for clang-tidy |
 | `make check_lint` | Run clang-tidy static analysis |
 | `make check_embed` | Syntax-check the embedded completion scripts |
-| `make clean` | Remove all build, test and docs artefacts |
+| `make clean` | Remove all build and docs artefacts |
 
 ## Requirements for a pull request
 
@@ -61,15 +63,20 @@ A PR automatically triggers the CI build workflow, which compiles and tests acro
 Run the test suite before submitting:
 
 ```sh
-make test_create_venv  # first-time setup only
-make test_mpqcli
+make test
+make test_functional
 ```
 
-All tests must pass without errors.
+All tests must pass without errors. A few functional tests need two real Blizzard patch archives and are skipped until you fetch them with `make fetch_integration_data`, which needs [filenget](https://github.com/thomaslaurenson/filenget). The building page explains more.
 
 ### 3. New features should include tests
 
-If your change adds or modifies user-facing functionality - such as a new subcommand flag or a change in output format - please include a corresponding test in the `test/` directory. The existing test files (`test_list.py`, `test_add.py`, etc.) are good references for the test style and fixtures used.
+If your change adds or modifies user-facing functionality - such as a new subcommand flag or a change in output format - please include a corresponding test under `test/`. The tests use Catch2 and come in two layers:
+
+- `test/unit/` mirrors `src/`, with one `test_<source>.cpp` per source file, and calls the core library directly. Prefer it for logic: it is faster, and a failure points at the code.
+- `test/functional/` has one file per subcommand and runs the built `mpqcli` binary, asserting on its exit code, stdout and stderr. Use it for what only the command line shows: argument parsing, exit codes and output.
+
+Shared fixtures live in `test/fixtures/`, one header each. The existing files, such as `test/functional/test_list.cpp` and `test/unit/util/test_path.cpp`, are good references for the style.
 
 ### 4. Linting must pass
 
@@ -89,7 +96,7 @@ Then re-run `make check_all` to confirm everything passes.
 
 ### 5. Match the existing code style
 
-C++ formatting is enforced by `.clang-format` (LLVM style base). Static analysis is enforced by `.clang-tidy`. Both configs live in the repo root. Python tests should follow the style of the existing test files.
+C++ formatting is enforced by `.clang-format` (LLVM style base). Static analysis is enforced by `.clang-tidy`. Both configs live in the repo root. The tests are held to the same formatting, but clang-tidy does not analyse them.
 
 #### Suppression policy
 
@@ -134,5 +141,5 @@ If you add a new StormLib call that is locale-sensitive, follow the existing pat
 3. Install the clang tools as shown above
 4. Make your changes and verify they build: `make build`
 5. Run `make check_all`, fixing any issues
-6. Run `make test_mpqcli` and confirm all tests pass
+6. Run `make test` and `make test_functional` and confirm all tests pass
 7. Open a pull request with a clear description of what was changed and why

Dockerfile

@@ -13,11 +13,13 @@ WORKDIR /src
 COPY . .
 
 # MPQCLI_BUILD_STATIC is what makes the scratch stage viable: the binary carries
-# musl and libstdc++ with it and needs no loader at runtime
+# musl and libstdc++ with it and needs no loader at runtime. The image ships the
+# binary alone, so the tests are not built.
 RUN cmake -B build/release \
         -DCMAKE_BUILD_TYPE=Release \
         -DMPQCLI_BUILD_APP=ON \
         -DMPQCLI_BUILD_STATIC=ON \
+        -DMPQCLI_BUILD_TESTING=OFF \
     && cmake --build build/release --parallel "$(nproc)" \
     && strip build/release/bin/mpqcli
 

Makefile

@@ -32,6 +32,15 @@ GCC_INSTALL_DIR  := $(shell f=$$(gcc -print-libgcc-file-name 2>/dev/null) && dir
 README           := README.md
 PACKAGE_URL      := https://github.com/thegraydot/mpqcli/pkgs/container/mpqcli
 
+# The real Blizzard patches a few functional tests read, which are never
+# committed. The default matches the MPQCLI_INTEGRATION_DATA cache variable in
+# CMakeLists.txt, so fetching them is all a test run needs. They are shared from
+# a Filen folder whose links decrypt client-side, so curl gets Filen's web app
+# rather than the files; filenget does the fetching.
+INTEGRATION_DATA ?= $(or $(MPQCLI_INTEGRATION_DATA),test/data/integration)
+INTEGRATION_LINK := https://drive.filen.io/f/cedf490b-aa38-406a-ad50-df35b1f0c5a9\#4t-jw8y2G9nK2igPN0tdVbty-lABHqDY
+FILENGET         ?= filenget
+
 # The version is declared once, in project(... VERSION X.Y.Z). The
 # cmake_minimum_required line also says VERSION and comes first, so it is skipped.
 VERSION_AWK      := /cmake_minimum_required/ { next } \
@@ -63,27 +72,27 @@ build: configure ## Build the project
 ##@ TEST
 
 .PHONY: test
-test: build test_mpqcli ## Run test suite (builds binary first)
+test: build ## Run Catch2 unit tests
+	ctest --test-dir build/dev --output-on-failure --parallel $(JOBS) -L unit
 
-.PHONY: test_create_venv
-test_create_venv: ## Create Python venv and install test dependencies
-	python3 -m venv ./.venv
-	. ./.venv/bin/activate && \
-	pip3 install -r test/requirements.txt
+.PHONY: test_verbose
+test_verbose: build ## Run unit tests with verbose Catch2 output
+	ctest --test-dir build/dev --verbose -L unit
 
-.PHONY: test_mpqcli
-test_mpqcli: ## Run pytest test suite
-	. ./.venv/bin/activate && \
-	python3 -m pytest test -s
+.PHONY: test_functional
+test_functional: build ## Run Catch2 functional tests against the built binary
+	ctest --test-dir build/dev --output-on-failure --parallel $(JOBS) -L functional
 
-.PHONY: test_clean
-test_clean: ## Remove test data directory
-	rm -rf test/data
+.PHONY: test_all
+test_all: build ## Run every test layer built into the current configure
+	ctest --test-dir build/dev --output-on-failure --parallel $(JOBS)
 
-.PHONY: test_lint
-test_lint: ## Run ruff linter on test directory
-	. ./.venv/bin/activate && \
-	ruff check ./test
+# The checksums pin the bytes the tests' expected values were taken from, so a
+# changed or truncated download fails here rather than as a puzzling test failure
+.PHONY: fetch_integration_data
+fetch_integration_data: ## Download the Blizzard patches some functional tests read (requires: filenget)
+	$(FILENGET) "$(INTEGRATION_DATA)" '$(INTEGRATION_LINK)'
+	cd "$(INTEGRATION_DATA)" && sha256sum --check "$(CURDIR)/test/integration_data.sha256"
 
 ##@ LINT
 
@@ -211,8 +220,10 @@ get_changelog: ## Print release notes for TAG to stdout (TAG=v1.0.0)
 check_all: check_format check_lint check_embed ## Run every static check
 
 .PHONY: ci
-ci: check_all test ## Run the checks CI runs
+ci: check_all test test_functional ## Run the checks CI runs
 
+# The fetched integration data is left alone: it is not build output, and only
+# a network fetch brings it back
 .PHONY: clean
-clean: test_clean docs_clean ## Remove all build, test, docs and release artefacts
+clean: docs_clean ## Remove all build, docs and release artefacts
 	rm -rf build dist install.sh install.ps1 checksums.txt checksums.txt.sigstore.json

app/create.cpp

@@ -51,6 +51,7 @@ void RegisterCreate(CLI::App &app, Context &context) {
     sub->add_option("--stream-flags", create.stream_flags, "Override stream flags")
         ->group("Game setting overrides");
     sub->add_option("--sector-size", create.sector_size, "Override sector size")
+        ->check(sector_size_valid)
         ->group("Game setting overrides");
     sub->add_option("--raw-chunk-size", create.raw_chunk_size, "Override raw chunk size for MPQ v4")
         ->group("Game setting overrides");