You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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 →
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::bigx → big::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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds additional test cases with original
BIGarhieves from Command & Conquer Generals Zero Hour to ensure that the extract works onBIGknown correct archieves.1. Commit (1bf6f2e):
Type of change