[lee3Q]

test/runjsontests.py builds the jsontestrunner command as one string and runs it through os.popen, so the shell splits and re-parses the executable path and options. A path that contains a space, or a quote character, is broken up. This change passes the command as an argument list to subprocess.Popen, with no shell.

What changes in test/runjsontests.py:

• getStatusOutput takes an argument list and uses subprocess.Popen. It returns the process return code (0 means success) and the decoded output. Its only caller is runAllTests, which only tests the value as true or false, so success and failure are still told apart the same way.
• The runner command (valgrind prefix, --parse-only, --strict, --json-writer, input path) is built as a list instead of a quoted string.
• If the executable cannot be started (missing or not executable), the OSError is printed to stderr and counted as a failed test, so the run continues and reports the failure like before instead of stopping with a traceback.

A new test/test_runjsontests.py (9 unittest cases) covers arguments with spaces and quotes, exit status and output, the option combinations, the valgrind prefix, and the missing/non-executable cases. It is registered in src/jsontestrunner/CMakeLists.txt (jsoncpp_runner_arguments) and in meson.build (runner_arguments).

Checked on Linux (Ubuntu 22.04, Python 3.10.12, GCC 11.4, CMake 3.22.1) at base 93acc7b:

• The 9 new tests pass with the change. Run against the unchanged base runner, the same tests give 4 failures, 4 errors and 1 pass.
• CMake, build directory path containing a space, Debug, static: the 4 test suites pass. CMake, normal path, Release, shared: the 4 test suites pass.
• Meson, build directory path containing a space, debug, static: all 4 tests including runner_arguments pass.

Not checked: Windows, macOS, Python 2 (the runner code keeps Python 2 syntax, but the new tests use Python 3 modules), and a real valgrind run (the valgrind prefix is only checked with a mock). clang-format was not run because no C++ file is touched. No C++ source or header changes.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

The PR runs jsontestrunner with an argument list instead of a shell command.

• The JSON test runner keeps paths and options intact when starting each test.

No actionable issues were found. This review checked the code without running the tests.

_{Reviews (1) · Last reviewed commit: "test: add unit tests for the jsontestrun..." · Reviewed by Greptile}