镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Keep CMake reader/writer test artifacts in the build tree - #1716

Open
vzer200 wants to merge 4 commits into
open-source-parsers:masterfrom
vzer200:fix/cmake-test-output-directory
Open

vzer200 wants to merge 4 commits into
open-source-parsers:masterfrom
vzer200:fix/cmake-test-output-directory

Conversation

@vzer200

@vzer200 vzer200 commented Sep 8, 2026 •

Copy link
Copy Markdown

Out-of-source CMake tests still write .actual, .rewrite, .actual-rewrite, and .process-output beside the source fixtures. With a read-only source tree, both reader/writer CTests fail while the unit test passes.

Add an optional runjsontests.py --output-dir and use separate build-tree directories for CTest (test-output/ctest) and jsoncpp_check (test-output/jsoncpp_check). The runner copies writer-test inputs there on each run, keeps expected results in the original source directory, and leaves parse-only fixtures in place. Manual calls without the option retain their existing behavior. Existing same-file inputs are recognized by file identity, including case aliases and hardlinks. Directory creation tolerates another caller creating that directory concurrently. The two reader/writer CTests retain their resource lock, and a concurrent custom-target run uses separate files. Multiple independently launched instances of the same entry point still require caller coordination or distinct output directories.

Fixes #1515.

Validation on Windows with LLVM-MinGW Clang 23, CMake 4.4.3, Ninja, and Python 3.12:

  • Built the unchanged baseline and final version, including all 132 C++ unit tests.
  • Reproduced the original failure with source fixture Write access denied: two reader/writer CTests failed, one unit CTest passed.
  • Independently reran all three final CTests with -j 3 and jsoncpp_check while writes to source fixtures were denied: all passed. Verified the denial was effective and all 401 fixture files retained their content and modification times; restored the original permissions afterward.
  • Independently exercised 17 real CLI scenarios: default manual behavior, omitted/single-file input, relative and space-containing output directories, parse-only cases, source expected-result authority, refreshed copied inputs, same-file aliases, and concurrent runs with separate output directories.
  • After the directory-creation review feedback, independently checked 20 deterministic two-caller creation races: the initial revision fails once per pair, while the final revision passes both calls. Existing output files still cause an error without modification. Reran the 17 CLI scenarios and all three parallel CTests on that final revision.
  • After the shared-output review feedback, reproduced the copied-input truncation window with a real runner, then independently ran three overlapping jsoncpp_check/CTest pairs with source writes denied. All six commands passed; both output directories had matching artifact contents, all 401 source fixtures were unchanged, and source permissions were restored.
  • git diff --check passed. No C++ library or ABI changes.

Prepared with Codex and reviewed by a separate automated agent. The review found two same-file alias failures in the initial implementation; both were corrected and the final revision was rechecked before submission. No human review is claimed. The read-only reproduction used Windows ACLs, not a Unix read-only mount.

@greptile-apps

greptile-apps Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Keeps CMake reader/writer test artifacts in the build tree.

  • CMake reader/writer tests keep generated files in the build tree.

Both earlier unnumbered findings are addressed. No new actionable issues were found.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Fixtures["Source fixtures"] --> Runner["runjsontests.py"]
  Runner --> CTest["test-output/ctest"]
  Runner --> Check["test-output/jsoncpp_check"]
  Expected["Source expected results"] --> Compare["Compare generated results"]
  CTest --> Compare
  Check --> Compare
Loading

Reviews (4) · Last reviewed commit: "Merge branch 'master' into fix/cmake-tes..." · Reviewed by Greptile

Comment thread test/runjsontests.py Outdated
Comment thread src/jsontestrunner/CMakeLists.txt Outdated
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 40347643828

Coverage remained the same at 89.926%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 2740
Covered Lines: 2615
Line Coverage: 95.44%
Relevant Branches: 2640
Covered Branches: 2223
Branch Coverage: 84.2%
Branches in Coverage %: Yes
Coverage Strength: 23835.16 hits per line

💛 - Coveralls

This branch has not been deployed

No deployments
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.

Tests write to source tree when invoked via CMake

3 participants