Skip to content

test: add test with game files - need to be added manually into test0…#2

Open
ViTeXFTW wants to merge 1 commit intomainfrom
test/game-files
Open

test: add test with game files - need to be added manually into test0…#2
ViTeXFTW wants to merge 1 commit intomainfrom
test/game-files

Conversation

@ViTeXFTW
Copy link
Copy Markdown
Owner

@ViTeXFTW ViTeXFTW commented Feb 6, 2026

Description

This PR adds additional test cases with original BIG arhieves from Command & Conquer Generals Zero Hour to ensure that the extract works on BIG known correct archieves.

1. Commit (1bf6f2e):

  • Added test03 which when used should be populated with developer game files.

Type of change

  • Bugfix
  • Feature
  • Formatting
  • Refactoring
  • Build
  • CI
  • Documentation
  • Tests
  • Other

@ViTeXFTW
Copy link
Copy Markdown
Owner Author

@claude

@claude
Copy link
Copy Markdown

claude Bot commented Feb 10, 2026

Claude finished @ViTeXFTW's task in 1m 16s —— View job


Code Review

  • Gather context and understand request
  • Read repository guidelines
  • Review changed files
  • Post review feedback

Overall this PR is well-structured. The skip-if-missing pattern for large test data is the right approach for files that can't be committed to git. A few issues worth addressing:


tests/test_w3d_archive.cpp

Issue 1 — Extension comparison is case-sensitive and brittle (lines 48–50, 148–150)

file.path.substr(file.path.size() - 4) != ".W3D"

This checks for uppercase .W3D only. BIG archives from C&C games can store file paths with lowercase .w3d as well. The safer approach is a case-insensitive comparison. The same pattern is repeated in CountArchiveFiles. Fix this →

Issue 2 — Path extraction duplicates logic (lines 56–65)

The manual slash-stripping to get the basename already exists in <filesystem>: fs::path(file.path).filename().string(). Using that would be shorter, handle both separators, and stay consistent with the rest of the code that already uses std::filesystem. Fix this →

Issue 3 — return after GTEST_SKIP() is redundant (lines 28–29, 135–136)

GTEST_SKIP() already stops the test body from continuing (it throws or longjmps internally). The return after it is a no-op and mildly misleading. This is a minor style issue—same pattern is used in the existing test_real_archive.cpp tests, so it's consistent if you want to keep it.

Issue 4 — missingReference count is never asserted or used in a decision (lines 43, 72–73, 115)

The test silently skips files with no reference counterpart and only reports the count in the summary. If the archive has many W3D files but zero reference files, verifiedCount will be 0 and EXPECT_GT(verifiedCount, 0) will fail—which is correct. However, missingReference being a large number while verifiedCount is also large would pass without comment, making it hard to understand what was actually validated. Consider logging the missing filenames at least at the DEBUG level, or adding an EXPECT_EQ(missingReference, 0) once reference files are populated.


.gitignore

Issue 5 — Missing newline at end of file

The diff shows \ No newline at end of file at the end of .gitignore. POSIX defines a text file as ending with a newline; most editors and tools expect this. Fix this →

Issue 6 — *.[wW]3[dD] ignores W3D files globally

This rule silently swallows any .w3d/.W3D file anywhere in the repository. If future utility scripts or sample data include a .w3d file outside tests/test03/, it would be ignored without warning. A more targeted rule like tests/test03/*.W3D (or the whole directory rule you already have) is sufficient. The global rule adds little value on top of tests/test03/.


tests/CMakeLists.txt

The bigx::bigxbig::big rename across all targets appears correct and consistent with the library's actual CMake target name. The addition of test_w3d_archive.cpp to archive_tests is appropriate.


Summary: The design (skip-if-missing, memory extraction, summary output) is sound. The main actionable items are the case-sensitive extension check (Issue 1) and the manual path-splitting that can be replaced with std::filesystem (Issue 2). The missing newline in .gitignore is trivial but worth fixing.
|

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