testing: add first component integration test to baselib#13
testing: add first component integration test to baselib#13saksham-chawla wants to merge 1 commit intocodex/replay-pr-51-basefrom
Conversation
initial version of CITs for baselibs
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid foundation for the new CIT framework, but pytest command handling, C++ input robustness, and docs/deps fixes are needed.
Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)
Issues Identified & Suggestions
- Fix marker args and cmd:
tests/python_test_cases/tests/conftest.py - Validate JSON parse, use public header, clean includes:
tests/cpp_test_scenarios/src/test_basic.cpp - Docs fix and pin Python dependency:
tests/README.md,tests/python_test_cases/requirements.txt
Review Details
📁 18 files reviewed | 💬 14 comments
👍 / 👎 individual comments to help improve reviews for you
| if marker.name not in markers_to_process: | ||
| continue | ||
|
|
||
| item.user_properties.append((marker.name, marker.args[0])) |
There was a problem hiding this comment.
[Logic] marker.args[0] will raise IndexError if a marker in markers_to_process is applied without arguments (e.g., @pytest.mark.Description with no args). Add a guard:
| if marker.name not in markers_to_process: | |
| continue | |
| item.user_properties.append((marker.name, marker.args[0])) | |
| if marker.name not in markers_to_process: | |
| continue | |
| if marker.args: | |
| item.user_properties.append((marker.name, marker.args[0])) |
Context for Agents
`marker.args[0]` will raise `IndexError` if a marker in `markers_to_process` is applied without arguments (e.g., `@pytest.mark.Description` with no args). Add a guard:
```suggestion
if marker.name not in markers_to_process:
continue
if marker.args:
item.user_properties.append((marker.name, marker.args[0]))
```
File: tests/python_test_cases/tests/conftest.py
Line: 172Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: While current usage always provides arguments to these markers (via add_test_properties), the markers are registered in pytest.ini without argument requirements. A future contributor could easily use @pytest.mark.Description without arguments, triggering an IndexError. This is plausible for a growing test suite.
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment correctly identifies a defensive coding issue in newly introduced code (this file is entirely new in this PR).
Line 172 does marker.args[0] which would raise IndexError if a marker in markers_to_process is used without arguments. In pytest, marker.args is a tuple, and accessing [0] on an empty tuple raises IndexError.
Current usage in test_basic.py uses add_test_properties decorator which likely adds markers with arguments, so the issue doesn't manifest today. However:
- The markers are registered in pytest.ini without any argument constraints
@pytest.mark.Description(no args) is valid pytest syntax- A future test could easily trigger this crash
The suggestion is concrete and actionable - adding if marker.args: guard before accessing marker.args[0]. This is standard defensive programming for new code and the fix is minimal and clear.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/test_basic.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/conftest.py - Lines:
157-172
157: def pytest_collection_modifyitems(items: list[pytest.Function]):
158: for item in items:
159: # Add custom markers info to XML report
160: for marker in item.iter_markers():
161: markers_to_process = (
162: "PartiallyVerifies",
163: "FullyVerifies",
164: "Description",
165: "TestType",
166: "DerivationTechnique",
...
3. File Snapshot (HEAD)
- File:
tests/python_test_cases/pytest.ini
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| # If bazel is used, modify command | ||
| if "BAZEL_TEST" in os.environ: | ||
| report.command = report.command.replace( | ||
| "tests/", | ||
| "bazel run //tests/", | ||
| ).replace( | ||
| " --name", |
There was a problem hiding this comment.
[Logic] The Bazel reproduction command you build here is almost always unusable. The original report.command starts with the absolute runfiles path of the C++ binary (e.g. /home/.../bazel-out/.../tests/cpp_test_scenarios/cpp_test_scenarios --name basic.basic --input …). Replacing every occurrence of the substring "tests/" with "bazel run //tests/" injects the Bazel invocation right into the middle of that absolute path (and into any JSON payload that happens to mention tests/), so the printed command cannot be executed. Instead of string replacements, construct the repro command explicitly from the known target label and the arguments, e.g.:
if "BAZEL_TEST" in os.environ:
target = item.session.config.getoption("--cpp-target-name")
args = " ".join(command[1:])
report.command = f"bazel run {target} -- {args}" if args else f"bazel run {target}"This guarantees a valid, copy-pastable command regardless of the absolute paths or payload contents.
Context for Agents
The Bazel reproduction command you build here is almost always unusable. The original `report.command` starts with the absolute runfiles path of the C++ binary (e.g. `/home/.../bazel-out/.../tests/cpp_test_scenarios/cpp_test_scenarios --name basic.basic --input …`). Replacing every occurrence of the substring `"tests/"` with `"bazel run //tests/"` injects the Bazel invocation right into the middle of that absolute path (and into any JSON payload that happens to mention `tests/`), so the printed command cannot be executed. Instead of string replacements, construct the repro command explicitly from the known target label and the arguments, e.g.:
```python
if "BAZEL_TEST" in os.environ:
target = item.session.config.getoption("--cpp-target-name")
args = " ".join(command[1:])
report.command = f"bazel run {target} -- {args}" if args else f"bazel run {target}"
```
This guarantees a valid, copy-pastable command regardless of the absolute paths or payload contents.
File: tests/python_test_cases/tests/conftest.py
Line: 128Chain of Thought
- Validation Decision: accepted
- Confidence: 0.88
- Likelihood Score: 4/5
- Likelihood Reason: When running under Bazel (BAZEL_TEST env set), the binary path will be an absolute runfiles path containing "tests/" as a substring within the path. The .replace() call will inject "bazel run //" into the middle of this absolute path, making the repro command unusable. This will happen every time the BAZEL_TEST code path is exercised.
- Suggested Action: keep
- Tool Calls Used: 12
Reasoning
The comment correctly identifies a real bug in newly added code (this file is entirely new in this PR).
Analysis of the string replacement logic (lines 123-130):
- The
commandlist is built from fixture tokens, where the first token is typically the path to the C++ binary executable. - Under Bazel, this path would be an absolute runfiles path like:
/home/user/.cache/bazel/.../bazel-out/k8-fastbuild/bin/tests/cpp_test_scenarios/cpp_test_scenarios - The full command string after joining would be:
/home/.../.../bin/tests/cpp_test_scenarios/cpp_test_scenarios --name basic.basic --input '...' .replace("tests/", "bazel run //tests/")replaces ALL occurrences of "tests/" in the string, including the one embedded in the absolute path, producing:/home/.../.../bin/bazel run //tests/cpp_test_scenarios/cpp_test_scenarios --name basic.basic --input '...'
This is clearly a broken command that cannot be executed.
Additionally, if any JSON payload argument contains the substring "tests/", that would also be corrupted by the replacement.
The suggested fix is concrete and well-reasoned:
- Uses
item.session.config.getoption("--cpp-target-name")which already has the correct default of"//tests/cpp_test_scenarios:cpp_test_scenarios"(defined at line 37) - Constructs the command from the known target label + arguments, avoiding any string replacement of arbitrary content
- The
command[1:]correctly skips the binary path and takes only arguments
The comment is accurate, identifies a real bug in new code, and provides an actionable, concrete fix.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/common.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Lookup
- Filename:
testing_utils - Max Results:
10
No files found matching 'testing_utils'
3. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/conftest.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
4. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/test_basic.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
5. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
6. File Lookup
- Filename:
BazelTools
No files found matching 'BazelTools'
7. File Lookup
- Filename:
testing_utils.py
No files found matching 'testing_utils.py'
8. File Snapshot (HEAD)
- File:
tests/python_test_cases/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
9. File Snapshot (HEAD)
- File:
MODULE.bazel
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| psutil | ||
| pytest-metadata | ||
| pytest-env | ||
| testing-utils @ git+https://github.com/eclipse-score/testing_tools.git@v0.3.0 |
There was a problem hiding this comment.
[Reliability] testing-utils is pinned to a mutable tag (@v0.3.0). For reproducible builds and supply-chain safety, pin to an immutable commit hash (or a released package version).
Context for Agents
`testing-utils` is pinned to a mutable tag (`@v0.3.0`). For reproducible builds and supply-chain safety, pin to an immutable commit hash (or a released package version).
File: tests/python_test_cases/requirements.txt
Line: 4Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: Git tags are mutable and can be force-pushed. While unlikely for a same-org repository (eclipse-score), the risk is real for supply-chain security. The auto-generated lock file itself warns about this exact issue (line 158-159: "WARNING: pip install will require the following package to be hashed. Consider using a hashable URL").
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment is valid and actionable:
-
Issue is real and introduced by this PR: The
requirements.txtfile is new (didn't exist at BASE). Line 4 pinstesting-utilsto a git tag@v0.3.0, which is indeed mutable - git tags can be moved/force-pushed. -
The lock file itself confirms the problem: The auto-generated
requirements.txt.lockcontains aWARNINGon lines 158-159 saying "pip install will require the following package to be hashed. Consider using a hashable URL like https://github.com/jazzband/pip-tools/archive/SOMECOMMIT.zip". This is pip-compile explicitly warning about the same issue the reviewer flagged. Thetesting-utilsentry in the lock file (line 160) is the only dependency without hash verification. -
Actionable: The suggestion is concrete - replace the tag reference with a commit hash (e.g.,
@<commit-sha>) to ensure immutability and allow hash verification. -
Mitigating factor: The repository (
eclipse-score/testing_tools) appears to be from the same organization, which somewhat reduces the supply-chain risk compared to an arbitrary third-party dependency. However, the best practice still applies, and the lock file's inability to hash this dependency is a concrete reproducibility concern. -
All other dependencies in the lock file have proper
--hash=sha256:...pinning, making this the only un-hashable dependency - a concrete gap in the integrity chain.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/python_test_cases/requirements.txt
1: psutil
2: pytest-metadata
3: pytest-env
4: testing-utils @ git+https://github.com/eclipse-score/testing_tools.git@v0.3.0
5:
2. File Snapshot (HEAD)
- File:
tests/python_test_cases/requirements.txt.lock
1: #
2: # This file is autogenerated by pip-compile with Python 3.12
3: # by the following command:
4: #
5: # bazel run //tests/python_test_cases:requirements.update
6: #
7: basedpyright==1.29.2 \
8: --hash=sha256:12c49186003b9f69a028615da883ef97035ea2119a9e3f93a00091b3a27088a6 \
9: --hash=sha256:f389e2997de33d038c5065fd85bff351fbdc62fa6d6371c7b947fc3bce8d437d
10: # via -r /home/pko/.cache/bazel/_bazel_pko/b37d285266e40b8446277261190db897/external/score_tooling+/python_basics/requirements.txt
...
| JsonParser parser; | ||
| auto data{parser.FromFile(params.json_path)}; | ||
|
|
||
| // String value extraction and tracing. | ||
| std::string string_key = "string_key"; | ||
| std::string value = data.value().As<Object>().value().get().at(string_key).As<std::string>().value(); | ||
| TRACING_INFO(kTargetName, std::pair{string_key, value}); |
There was a problem hiding this comment.
[Reliability] The JSON file loaded in BasicScenario::run is never validated before calling .value(). JsonParser::FromFile returns a score::Result, as do the chained As<Object>()/As<std::string>() calls, but the current code dereferences every result unconditionally. If the input path is wrong or the JSON doesn't have the expected shape, value() will throw an unhelpful std::bad_variant_access (since the underlying std::variant holds the error alternative), making failures very hard to diagnose. Note that map_to_params already checks its initial parse result — the same pattern should be applied here. Please check each result and raise a descriptive exception before using the values:
JsonParser parser;
auto data = parser.FromFile(params.json_path);
if (!data)
{
throw std::runtime_error("Failed to read JSON file: " + params.json_path);
}
auto root = data.value().As<Object>();
if (!root)
{
throw std::runtime_error("Root JSON value is not an object");
}
const auto &object = root.value().get();Apply the same pattern to the later As<std::string>() / As<int>() calls so bad input yields a controlled error instead of an obscure exception.
Context for Agents
The JSON file loaded in `BasicScenario::run` is never validated before calling `.value()`. `JsonParser::FromFile` returns a `score::Result`, as do the chained `As<Object>()`/`As<std::string>()` calls, but the current code dereferences every result unconditionally. If the input path is wrong or the JSON doesn't have the expected shape, `value()` will throw an unhelpful `std::bad_variant_access` (since the underlying `std::variant` holds the error alternative), making failures very hard to diagnose. Note that `map_to_params` already checks its initial parse result — the same pattern should be applied here. Please check each result and raise a descriptive exception before using the values:
```c++
JsonParser parser;
auto data = parser.FromFile(params.json_path);
if (!data)
{
throw std::runtime_error("Failed to read JSON file: " + params.json_path);
}
auto root = data.value().As<Object>();
if (!root)
{
throw std::runtime_error("Root JSON value is not an object");
}
const auto &object = root.value().get();
```
Apply the same pattern to the later `As<std::string>()` / `As<int>()` calls so bad input yields a controlled error instead of an obscure exception.
File: tests/cpp_test_scenarios/src/test_basic.cpp
Line: 72Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: In a test scenario context, wrong JSON paths or unexpected JSON shapes are plausible (e.g., during development, config changes, or CI setup issues). The unchecked .value() calls would produce unhelpful std::bad_variant_access errors rather than clean diagnostics. However, since this is controlled test infrastructure with presumably well-known inputs, it's plausible but not inevitable.
- Suggested Action: modify
- Tool Calls Used: 11
Reasoning
The comment correctly identifies that BasicScenario::run() never validates the score::Result returned by parser.FromFile() before calling .value() on it (line 67→71, 76). This is genuinely new code introduced by this PR. The suggestion to add validation is reasonable and actionable, with concrete code provided.
However, the comment's claim that "the score::Result will std::terminate" is technically inaccurate. The value() method in the expected class implementation (expected.h:502-505) calls std::get<0>(storage_), which throws std::bad_variant_access when the variant holds an error. Since neither value() nor run() are marked noexcept, this exception propagates rather than causing std::terminate. The resulting std::bad_variant_access is still an unhelpful error message, but it's not an abrupt process termination.
The inconsistency within the file is notable: map_to_params() checks the initial parse result (line 36: if (!any_res)), but run() doesn't check data at all. And even in map_to_params(), the subsequent .As<Object>().value() chains (lines 40-41, 45) are unchecked.
The core recommendation is valid and actionable: adding Result checks before .value() calls would produce descriptive error messages instead of obscure std::bad_variant_access exceptions. I'll modify the comment to correct the technical inaccuracy about std::terminate.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Lookup
- Filename:
json_parser.h
Found 1 file(s) matching 'json_parser.h':
- score/json/json_parser.h
3. File Snapshot (HEAD)
- File:
score/json/json_parser.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Lookup
- Filename:
result.h - Max Results:
10
Found 1 file(s) matching 'result.h':
- score/result/result.h
5. File Snapshot (HEAD)
- File:
score/result/result.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
6. File Lookup
- Filename:
expected.h - Max Results:
10
Found 1 file(s) matching 'expected.h':
- score/result/details/expected/expected.h
7. File Snapshot (HEAD)
- File:
score/result/details/expected/expected.h - Lines:
1-100
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Snapshot (HEAD)
- File:
score/result/details/expected/expected.h - Lines:
300-500
300: : impl::base_expected{}, storage_{std::in_place_index<1>, std::forward<const G&>(error.error())}
301: {
302: }
303:
304: template <typename G, typename = std::enable_if_t<std::is_constructible_v<E, G>>>
305: // DIVERGENCE FROM STANDARD: Explicitness cannot be specified according to standard, since this requires a C++20
306: // feature. Anyway, only available if E can be implicitly constructed from G.
307: // NOLINTNEXTLINE(google-explicit-constructor) see justification above
308: constexpr expected(unexpected<G>&& error)
309: : impl::base_expected{}, storage_{std::in_place_index<1>, std::forward<G>(error.error())}
...
9. File Snapshot (HEAD)
- File:
score/result/details/expected/expected.h - Lines:
500-600
500:
501: // DIVERGENCE FROM STANDARD: Added [[nodiscard]] for better safety
502: [[nodiscard]] constexpr T& value() &
503: {
504: return std::get<0>(storage_);
505: }
506: // DIVERGENCE FROM STANDARD: Added [[nodiscard]] for better safety
507: [[nodiscard]] constexpr T&& value() &&
508: {
509: return std::get<0>(std::move(storage_));
...
| - `-v` - increase verbosity. | ||
| - `-s` - show logs (disable capture). | ||
| - `-k <PATTERN>` - run tests matching the pattern. | ||
| - `--build-scenarios` - build Rust test scenarios before execution. |
There was a problem hiding this comment.
[Documentation] Documentation error: the README says --build-scenarios will "build Rust test scenarios before execution", but the PR introduces C++ test scenarios. This will confuse users.
| - `--build-scenarios` - build Rust test scenarios before execution. | |
| - `--build-scenarios` - build C++ test scenarios before execution. |
Context for Agents
Documentation error: the README says `--build-scenarios` will "build **Rust** test scenarios before execution", but the PR introduces **C++** test scenarios. This will confuse users.
```suggestion
- `--build-scenarios` - build C++ test scenarios before execution.
```
File: tests/README.md
Line: 40Chain of Thought
- Validation Decision: accepted
- Confidence: 0.98
- Likelihood Score: 5/5
- Likelihood Reason: The documentation error is definite - the README says "Rust" but the entire PR introduces C++ test scenarios. Anyone reading the docs will be confused.
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment correctly identifies a documentation error introduced by this PR. Line 40 of tests/README.md states --build-scenarios will "build Rust test scenarios before execution", but:
- The conftest.py (lines 72-73) explicitly refers to "Building C++ test scenarios executable..."
- The
--cpp-target-nameoption defaults to//tests/cpp_test_scenarios:cpp_test_scenarios - The PR introduces C++ test scenarios in
tests/cpp_test_scenarios/with.cppfiles - The rest of the README itself has a "### C++ Scenarios" section (line 94)
- There are no Rust test scenarios anywhere in this PR
This is a clear copy-paste error where "Rust" should be "C++". The issue is introduced by this PR (the README is newly created) and will definitely confuse users.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/README.md - Lines:
38-42
38: - `-s` - show logs (disable capture).
39: - `-k <PATTERN>` - run tests matching the pattern.
40: - `--build-scenarios` - build Rust test scenarios before execution.
41:
42: Run tests repeatedly:
2. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/conftest.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
3. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/main.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| Run test scenario executable directly: | ||
|
|
||
| ```bash | ||
| <REPO_ROOT>/bazel-bin/tests/cpp_test_scenarios/cpp_test_scenarios --name basic.basic --input '{"kvs_parameters":{"instance_id":0}}' |
There was a problem hiding this comment.
[Documentation] The example under "Run specific test scenario" (line 117) shows the correct input:
--input '{"test_logic":{"json_path":"data.json"}}'
but the "Run test scenario executable directly" example (line 123) switches to {"kvs_parameters":{"instance_id":0}}. The C++ scenario's map_to_params hard-codes access to test_logic.json_path (see test_basic.cpp line 40), so using kvs_parameters will cause at("test_logic") to throw at runtime. Please update line 123 to use the same test_logic structure so users can run the binary directly without hitting an immediate error.
Context for Agents
The example under "Run specific test scenario" (line 117) shows the correct input:
```
--input '{"test_logic":{"json_path":"data.json"}}'
```
but the "Run test scenario executable directly" example (line 123) switches to `{"kvs_parameters":{"instance_id":0}}`. The C++ scenario's `map_to_params` hard-codes access to `test_logic.json_path` (see `test_basic.cpp` line 40), so using `kvs_parameters` will cause `at("test_logic")` to throw at runtime. Please update line 123 to use the same `test_logic` structure so users can run the binary directly without hitting an immediate error.
File: tests/README.md
Line: 123Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 5/5
- Likelihood Reason: Anyone copying the second example verbatim will inevitably hit a runtime error because the input JSON structure doesn't match what map_to_params expects.
- Suggested Action: modify
- Tool Calls Used: 3
Reasoning
The comment correctly identifies a real documentation inconsistency introduced by this PR (the entire README is new).
Evidence:
- Line 117 example uses
{"test_logic":{"json_path":"data.json"}}- correct - Line 123 example uses
{"kvs_parameters":{"instance_id":0}}- incorrect for this scenario - Line 130 (GDB example) uses
{"test_logic":{"json_path":"data.json"}}- correct
In test_basic.cpp, map_to_params() explicitly looks for "test_logic" key (line 40) and then "json_path" within it (line 45). Passing {"kvs_parameters":{"instance_id":0}} would cause at("test_logic") to throw std::out_of_range, not "Failed to parse JSON data" as the comment states (that message is only for JSON parse failures at line 38). However, the core issue is accurate: the second example would fail at runtime.
The comment is actionable - it clearly identifies the specific line and suggests updating the command to use the test_logic structure. This is a new file introduced by the PR, so it's clearly PR-introduced.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.hpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/main.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| return logs.get_logs(field="target", pattern=f"{target_path.name}.*") | ||
|
|
||
| @pytest.fixture(scope="class") | ||
| def logs_info_level(self, logs_target: LogContainer) -> LogContainer: |
There was a problem hiding this comment.
[Unused Code] The fixture logs_info_level is defined here but, after searching every file in this PR, it isn’t requested anywhere. Because it’s not marked autouse, pytest will never execute it; consider removing it or wiring it up where needed.
Context for Agents
The fixture `logs_info_level` is defined here but, after searching every file in this PR, it isn’t requested anywhere. Because it’s not marked `autouse`, pytest will never execute it; consider removing it or wiring it up where needed.
File: tests/python_test_cases/tests/common.py
Line: 103Chain of Thought
- Validation Decision: accepted
- Confidence: 0.70
- Likelihood Score: 3/5
- Likelihood Reason: The fixture is genuinely unused in this PR. However, CitScenario is clearly a reusable base class, and this fixture may be intentionally included for future subclasses. The "dead code" concern is valid but moderate since it's part of a base class API.
- Suggested Action: modify
- Tool Calls Used: 4
Reasoning
The comment is factually correct: logs_info_level is defined as a pytest fixture at line 103 in CitScenario, it's not marked autouse, and it is not requested by any test method or fixture in the entire PR (verified by examining test_basic.py and conftest.py).
However, CitScenario is clearly designed as a reusable base class for component integration tests. It provides multiple convenience fixtures (build_tools, temp_dir, logs_target, logs_info_level, print_to_report) for subclasses. The logs_info_level fixture follows the same pattern as logs_target — filtering the log container by a specific field. It's reasonable that this fixture was intentionally provided as part of the base class API for future test classes that may need INFO-level log filtering.
That said, the comment is actionable and identifies real dead code in the current PR. The suggestion to "remove it or wire it up where needed" gives the developer a clear action. The issue is introduced by this PR (new file).
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/test_basic.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Snapshot (HEAD)
- File:
tests/python_test_cases/tests/conftest.py
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| #include <optional> | ||
| #include <string> |
There was a problem hiding this comment.
[Maintainability] Unused #include <optional> — BasicScenario does not use std::optional. Remove to keep includes clean.
| #include <optional> | |
| #include <string> | |
| #include <string> |
Context for Agents
Unused `#include <optional>` — `BasicScenario` does not use `std::optional`. Remove to keep includes clean.
```suggestion
#include <string>
```
File: tests/cpp_test_scenarios/src/test_basic.hpp
Line: 16Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: The
#include <optional>is clearly unused in this newly-added header file —BasicScenarioonly usesstd::string. The unnecessary include will persist indefinitely unless removed. - Suggested Action: keep
- Tool Calls Used: 7
Reasoning
The comment claims #include <optional> is unused in test_basic.hpp and should be removed. Evidence:
-
File is newly added by this PR — the diff shows 0 lines before and 28 lines after, all "added". This is not pre-existing debt.
-
Header file analysis:
BasicScenariodeclares only:std::string name() const final;void run(const std::string &input) const final;~BasicScenario() final = default;
Nostd::optionaltype is used anywhere in the header.
-
Implementation file analysis (
test_basic.cpp): While the .cpp file uses.value()on results from a JSON parser, those are the parser's own result types, notstd::optionalfrom the standard library header. Moreover, the .cpp doesn't rely on<optional>being transitively included from the header — it has its own includes. -
scenario.hppnot found in the repo (possibly external or in a different build target), so there's a small chance it could depend on<optional>being included before it. However, well-structured headers should include their own dependencies, making this unlikely.
The comment is accurate, actionable (provides a concrete suggestion to remove the line), and addresses code introduced by this PR. It's a valid include hygiene fix.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Lookup
- Filename:
scenario.hpp
No files found matching 'scenario.hpp'
3. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.hpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| ********************************************************************************/ | ||
| #include "test_basic.hpp" | ||
|
|
||
| #include <cassert> |
There was a problem hiding this comment.
[Maintainability] #include <cassert> isn’t used in this file. Remove it to reduce compile dependencies.
Context for Agents
`#include <cassert>` isn’t used in this file. Remove it to reduce compile dependencies.
File: tests/cpp_test_scenarios/src/test_basic.cpp
Line: 15Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: The
assertmacro from<cassert>is definitively not used anywhere in the file. This is a brand new file added in this PR, so the unused include is certainly introduced by this PR. - Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment claims #include <cassert> is unused in test_basic.cpp. I verified the full file content at HEAD:
- The file is entirely new (added in this PR - the diff shows before_change_line_count=0).
<cassert>provides theassert()macro.- Searching through all 78 lines of the file, there is zero usage of
assert(...)anywhere. - The header file
test_basic.hppalso does not include<cassert>.
The comment is accurate, actionable (remove the include), and targets code introduced by this PR. It's a straightforward cleanup to remove an unused include in newly added code.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.hpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
|
|
||
| #include <cassert> | ||
| #include <iostream> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
[Maintainability] #include <unordered_map> isn’t used in this file. Remove it to reduce compile dependencies.
Context for Agents
`#include <unordered_map>` isn’t used in this file. Remove it to reduce compile dependencies.
File: tests/cpp_test_scenarios/src/test_basic.cpp
Line: 17Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 5/5
- Likelihood Reason: The include is definitively unused - the Object type is std::map, not std::unordered_map, and no code in the file references std::unordered_map. This is dead code that will persist unless removed.
- Suggested Action: keep
- Tool Calls Used: 9
Reasoning
The comment correctly identifies that #include <unordered_map> on line 17 of test_basic.cpp is unused. Evidence:
- File is new in this PR: The diff shows 0→78 lines, so this is entirely PR-introduced code.
- No direct usage: Scanning through all 78 lines of the file, there is no mention of
std::unordered_mapor any type alias that depends on it. - JSON Object type uses std::map: The
Objecttype fromany.hline 38 is defined asusing Object = std::map<score::memory::StringComparisonAdaptor, score::json::Any>;— this isstd::map, notstd::unordered_map. - All
.at()calls operate on std::map or std::string: Lines 40, 41, 45, 71, 76 call.at()onObject(std::map) instances. - Not needed transitively: The included headers (
json_parser.h,any.h, etc.) use<map>not<unordered_map>.
The comment is accurate, actionable (simply remove the line), and addresses code introduced by this PR.
Evidence Summary
1. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
tests/cpp_test_scenarios/src/test_basic.hpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Lookup
- Filename:
tracing.hpp
No files found matching 'tracing.hpp'
4. File Lookup
- Filename:
json_parser.h
Found 1 file(s) matching 'json_parser.h':
- score/json/json_parser.h
5. File Snapshot (HEAD)
- File:
score/json/json_parser.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
6. File Lookup
- Filename:
i_json_parser.h
Found 1 file(s) matching 'i_json_parser.h':
- score/json/i_json_parser.h
7. File Lookup
- Filename:
any.h - Max Results:
10
Found 1 file(s) matching 'any.h':
- score/json/internal/model/any.h
8. File Snapshot (HEAD)
- File:
score/json/internal/model/any.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2 participants
initial version of CITs for baselibs
testing: add first component integration test to baselib
This pull request introduces the initial set of Component Integration Tests (CITs) for the
baselibsproject. It establishes a Python-basedpytestframework to orchestrate and execute C++ test scenarios, specifically demonstrating basic JSON deserialization. The changes include new C++ test code, Python test cases,pytestconfiguration, and updates to the Bazel build system to support this new testing paradigm.Architecture Diagram:
flowchart TD subgraph Python_Testing_Layer PT["Python_Tests (tests/python_test_cases/tests/test_basic.py +71)"] PC[/"Pytest_Config (tests/python_test_cases/tests/conftest.py +172, tests/python_test_cases/pytest.ini +22)"/] PT -->|uses| PC end subgraph C++_Scenario_Layer CSA["C++_Scenarios_App (tests/cpp_test_scenarios/src/main.cpp +45)"] BS["Basic_Scenario (tests/cpp_test_scenarios/src/test_basic.hpp +28, tests/cpp_test_scenarios/src/test_basic.cpp +78)"] CSA -->|includes| BS BS -->|parses with| JP BS -->|reads| TD end subgraph Build_System BB["Bazel_Build (BUILD +5, tests/python_test_cases/BUILD +58, tests/cpp_test_scenarios/BUILD +31, MODULE.bazel +23, .bazelrc +3)"] end BB -->|builds| PT BB -->|builds| CSA PT -->|executes| CSA PT -->|uses| TU TU[/"Testing_Utils (dependency)"/] JP[/"JSON_Parser (existing lib)"/] TD[("Test_Data (temp file)")]This summary was automatically generated by @propel-code-bot