-
Notifications
You must be signed in to change notification settings - Fork 0
Adds datarouter integration test #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: codex/replay-pr-33-base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2025 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| load("@score_itf//:defs.bzl", "py_itf_test") | ||
|
|
||
| package(default_testonly = True) | ||
|
|
||
| py_itf_test( | ||
| name = "test_datarouter_dlt", | ||
| srcs = [ | ||
| "__init__.py", | ||
| "conftest.py", | ||
| "test_datarouter_dlt.py", | ||
| ], | ||
| args = [ | ||
| "--ecu=s_core_ecu_qemu_bridge_network", | ||
| "--target_config=$(location @score_itf//config)", | ||
| "--hw", # Required even if we run the test against a QEMU target | ||
| ], | ||
| data = [ | ||
| "@score_itf//config", | ||
| ], | ||
| plugins = [ | ||
| "itf.plugins.base.base_plugin", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||
| # ******************************************************************************* | ||||||
| # Copyright (c) 2025 Contributors to the Eclipse Foundation | ||||||
| # | ||||||
| # See the NOTICE file(s) distributed with this work for additional | ||||||
| # information regarding copyright ownership. | ||||||
| # | ||||||
| # This program and the accompanying materials are made available under the | ||||||
| # terms of the Apache License Version 2.0 which is available at | ||||||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||||||
| # | ||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||
| # ******************************************************************************* | ||||||
|
|
||||||
| # Integration Tests | ||||||
|
|
||||||
| Integration tests for the `score_logging` component that verify DLT (Diagnostic Log and Trace) message capture on a QNX 8.0 QEMU target. | ||||||
|
|
||||||
| ## Prerequisites | ||||||
|
|
||||||
| Complete the QNX 8.0 QEMU environment setup as described in the [QNX QEMU documentation](score_reference_integration/qnx_qemu/README.md). | ||||||
|
|
||||||
| ### Network Bridge Setup (virbr0) | ||||||
|
|
||||||
| The integration tests use QEMU with bridge networking, which requires the `virbr0` virtual bridge interface. Instructions: reference_integration/qnx_qemu/README.md#Configure QEMU networking | ||||||
|
|
||||||
| This creates: | ||||||
| - Bridge interface `virbr0` with IP `192.168.122.1/24` on the host. Reference: /usr/share/libvirt/networks/default.xml | ||||||
| - DHCP server assigning IPs `192.168.122.2-254` to VMs | ||||||
| - NAT rules for outbound internet access | ||||||
|
|
||||||
| ## Usage | ||||||
|
|
||||||
| ### 1. Start the QNX QEMU Target | ||||||
|
|
||||||
| Build and launch the QEMU virtual machine from the `score_reference_integration/qnx_qemu` directory: | ||||||
|
|
||||||
| ```bash | ||||||
| bazel build --config=x86_64-qnx \ | ||||||
| --credential_helper=*.qnx.com=$(pwd)/../toolchains_qnx/tools/qnx_credential_helper.py \ | ||||||
| //build:init | ||||||
|
|
||||||
| bazel run --config=x86_64-qnx //:run_qemu | ||||||
| ``` | ||||||
|
|
||||||
| Note the assigned IP address once the QEMU VM boots (e.g., `192.168.122.76`) since the actual IP address of the target (QEMU VM) is assigned dynamically via DHCP hence changes every reboot. We use the S_CORE_ECU_QEMU_BRIDGE_NETWORK_PP target config for the tests. | ||||||
|
|
||||||
| ### 2. Execute the Tests | ||||||
|
|
||||||
| Run the integration tests from the `score_logging` directory, specifying the target IP: | ||||||
|
|
||||||
| ```bash | ||||||
| bazel test //tests/integration:test_datarouter_dlt \ | ||||||
| --test_arg=--target-ip=<QEMU_VM_IP> \ | ||||||
| --test_output=streamed \ | ||||||
| --nocache_test_results | ||||||
| ``` | ||||||
|
|
||||||
| ## Test Details | ||||||
|
|
||||||
| ### test_datarouter_dlt | ||||||
|
|
||||||
| Validates DLT message capture from the datarouter by: | ||||||
|
|
||||||
| 1. Connecting to the QNX target via SSH | ||||||
| 2. Starting the datarouter process if not already running | ||||||
| 3. Capturing DLT messages over UDP for approximately 10 seconds | ||||||
| 4. Verifying receipt of messages with `APP_ID=DR` and `CTX_ID=STAT` | ||||||
|
|
||||||
| The datarouter emits statistics messages every 5 seconds. The test expects at least 1 message during the capture window for reliable test results | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Documentation] The documentation states the test expects "at least 1 message", but the implementation in
Suggested change
Context for AgentsChain of Thought
Reasoning The comment correctly identifies a documentation-code mismatch introduced in this PR:
"At least 1" corresponds to Evidence Summary 1. File Snapshot (HEAD)
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2025 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,86 @@ | ||||||||||||||||||||||||||||||
| # ******************************************************************************* | ||||||||||||||||||||||||||||||
| # Copyright (c) 2025 Contributors to the Eclipse Foundation | ||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||
| # See the NOTICE file(s) distributed with this work for additional | ||||||||||||||||||||||||||||||
| # information regarding copyright ownership. | ||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||
| # This program and the accompanying materials are made available under the | ||||||||||||||||||||||||||||||
| # terms of the Apache License Version 2.0 which is available at | ||||||||||||||||||||||||||||||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||||
| # ******************************************************************************* | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from itf.plugins.com.ssh import execute_command, execute_command_output | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def pytest_addoption(parser): | ||||||||||||||||||||||||||||||
| parser.addoption( | ||||||||||||||||||||||||||||||
| "--target-ip", | ||||||||||||||||||||||||||||||
| action="store", | ||||||||||||||||||||||||||||||
| required=True, | ||||||||||||||||||||||||||||||
| help="Target IP address for SSH connection to target (QEMU VM)", | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @pytest.hookimpl(trylast=True) | ||||||||||||||||||||||||||||||
| def pytest_sessionstart(session): | ||||||||||||||||||||||||||||||
| """Override test target config IP with the current target (QEMU VM) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| We use the S_CORE_ECU_QEMU_BRIDGE_NETWORK_PP target config | ||||||||||||||||||||||||||||||
| for the tests, but the actual IP address of the target (QEMU VM) is | ||||||||||||||||||||||||||||||
| assigned dynamically via DHCP. This hook updates the target | ||||||||||||||||||||||||||||||
| configuration for the test session. | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| from itf.plugins.base.base_plugin import TARGET_CONFIG_KEY | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if TARGET_CONFIG_KEY in session.stash: | ||||||||||||||||||||||||||||||
| target_ip_address = session.config.getoption("--target-ip") | ||||||||||||||||||||||||||||||
| target_config = session.stash[TARGET_CONFIG_KEY] | ||||||||||||||||||||||||||||||
| target_config._BaseProcessor__ip_address = target_ip_address | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Maintainability] The hook rewrites Context for AgentsChain of Thought
Reasoning The comment correctly identifies a real anti-pattern introduced by this PR:
The only limitation is that without access to the score_itf library source, we can't verify exactly what public alternatives exist. However, the fundamental concern about accessing name-mangled private attributes of an external dependency is well-founded and actionable. Evidence Summary 1. File Lookup
2. File Snapshot (HEAD)
3. File Snapshot (HEAD)
4. File Lookup
|
||||||||||||||||||||||||||||||
| logger.info("Connecting to target IP: %s", target_ip_address) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| _DATAROUTER_CHECK_CMD = "/proc/boot/pidin | /proc/boot/grep datarouter" | ||||||||||||||||||||||||||||||
| # pathspace ability provides the datarouter access to the `procnto` pathname prefix space | ||||||||||||||||||||||||||||||
| # required for mw/com message passing with mw::log frontend | ||||||||||||||||||||||||||||||
| _DATAROUTER_START_CMD = ( | ||||||||||||||||||||||||||||||
| "cd /usr/bin && on -A nonroot,allow,pathspace -u 1051:1091 " | ||||||||||||||||||||||||||||||
| "./datarouter --no_adaptive_runtime &" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| _DATAROUTER_STARTUP_TIMEOUT_SEC = 2 | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @pytest.fixture(scope="module") | ||||||||||||||||||||||||||||||
| def datarouter_running(target_fixture): | ||||||||||||||||||||||||||||||
| with target_fixture.sut.ssh() as ssh: | ||||||||||||||||||||||||||||||
| exit_status, stdout_lines, _ = execute_command_output(ssh, _DATAROUTER_CHECK_CMD) | ||||||||||||||||||||||||||||||
| output = "\n".join(stdout_lines) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if "datarouter" not in output: | ||||||||||||||||||||||||||||||
| logger.info("Datarouter not running. Starting Datarouter..") | ||||||||||||||||||||||||||||||
| execute_command(ssh, _DATAROUTER_START_CMD) | ||||||||||||||||||||||||||||||
| time.sleep(_DATAROUTER_STARTUP_TIMEOUT_SEC) | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Testing] Using a fixed
Suggested change
Context for AgentsChain of Thought
Reasoning The comment targets line 69 in a brand-new file introduced by this PR ( Accuracy: The comment correctly identifies the fixed sleep pattern and its potential for flakiness. The current code does have a verification check after the sleep (lines 71-73) that will fail the test if the process hasn't started, but this means if 2 seconds isn't long enough, the test will fail unnecessarily rather than retrying. Actionability: The suggestion provides a complete, concrete code replacement using a polling loop with configurable timeout (10 seconds) and poll interval (0.5 seconds). This is highly actionable. Diff Responsibility: This is entirely new code introduced by this PR, so the issue is squarely introduced by this change. Technical merit: The polling approach is strictly better because:
The context that this runs against a QEMU VM (as noted in the code's own comments) makes variable startup times more likely, strengthening the comment's concern. Evidence Summary 1. File Snapshot (HEAD)
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| _, stdout_lines, _ = execute_command_output(ssh, _DATAROUTER_CHECK_CMD) | ||||||||||||||||||||||||||||||
| if "datarouter" not in "\n".join(stdout_lines): | ||||||||||||||||||||||||||||||
| pytest.fail("Failed to start datarouter on target") | ||||||||||||||||||||||||||||||
| logger.info("Datarouter started successfully..") | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| logger.info("Datarouter already running!") | ||||||||||||||||||||||||||||||
| yield | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @pytest.fixture(scope="module") | ||||||||||||||||||||||||||||||
| def dlt_receiver_config(target_config_fixture): | ||||||||||||||||||||||||||||||
| data_router_config = target_config_fixture.data_router_config | ||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||
| "vlan_address": data_router_config["vlan_address"], | ||||||||||||||||||||||||||||||
| "multicast_addresses": data_router_config["multicast_addresses"], | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2025 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| import logging | ||
| import os | ||
| import time | ||
|
|
||
| from itf.plugins.dlt.dlt_receive import DltReceive, Protocol | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Datarouter messages with Context ID "STAT" sent every ~5s hence 10s to reliably capture and verify | ||
| CAPTURE_DURATION_SECONDS = 10 | ||
|
|
||
| # DLT message identifiers for datarouter statistics | ||
| APP_ID = "DR" | ||
| CTX_ID = "STAT" | ||
|
|
||
|
|
||
| def test_dlt_capture(datarouter_running, test_config_fixture, dlt_receiver_config): | ||
| """Verify DLT messages can be captured from the datarouter. | ||
| """ | ||
| dlt_file = "/tmp/test_dlt_capture.dlt" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Testing] The test uses a hardcoded file path Example: # In test function signature:
def test_dlt_capture(..., tmp_path):
# In test body:
dlt_file = tmp_path / "test_dlt_capture.dlt"
# In DltReceive call:
with DltReceive(
...,
file_name=str(dlt_file),
...
):Context for AgentsChain of Thought
Reasoning The comment correctly identifies that the newly added test file uses a hardcoded path Accuracy: The comment accurately describes the issue. The hardcoded
Actionability: The comment is highly actionable, providing:
Considerations: This is a Bazel-based integration test ( Diff Responsibility: This is newly introduced code in this PR, so the issue is squarely within the PR's responsibility. Evidence Summary 1. File Snapshot (HEAD)
2. File Snapshot (HEAD)
|
||
|
|
||
| vlan_address = dlt_receiver_config["vlan_address"] | ||
| multicast_addresses = dlt_receiver_config["multicast_addresses"] | ||
|
|
||
| # TODO: Replace with DltWindow when fixed in ITF. | ||
| with DltReceive( | ||
| target_ip=vlan_address, | ||
| protocol=Protocol.UDP, | ||
| file_name=dlt_file, | ||
| binary_path=test_config_fixture.dlt_receive_path, | ||
| data_router_config={ | ||
| "vlan_address": vlan_address, | ||
| "multicast_addresses": multicast_addresses, | ||
| }, | ||
| ): | ||
| time.sleep(CAPTURE_DURATION_SECONDS) | ||
|
|
||
| assert os.path.exists(dlt_file), f"DLT file not created: {dlt_file}" | ||
|
|
||
| with open(dlt_file, "rb") as f: | ||
| dlt_data = f.read() | ||
|
|
||
| logger.info("DLT file size: %d bytes", len(dlt_data)) | ||
|
|
||
| # DLT extended header: APP-ID (4 bytes) followed by CTX-ID (4 bytes) | ||
| pattern = f"{APP_ID}\x00\x00{CTX_ID}".encode() | ||
| message_count = dlt_data.count(pattern) | ||
|
|
||
| logger.info("Found %d messages with app_id=%s, context_id=%s", message_count, APP_ID, CTX_ID) | ||
|
|
||
| assert message_count > 1, f"Expected more than 1 message, but got {message_count}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Logic] The README and docstring state that the test only needs to observe at least one - assert message_count > 1, f"Expected more than 1 message, but got {message_count}"
+ assert message_count >= 1, f"Expected at least 1 message, but got {message_count}"(If a higher threshold is really required, update the documentation and fixture comments accordingly.) Context for AgentsChain of Thought
Reasoning The comment accurately identifies a real inconsistency between the documentation and the code:
With a 5-second emission interval and a 10-second capture window, the test could realistically capture 1 or 2 messages depending on when the capture starts relative to the emission cycle. If capture starts just after an emission, the next one arrives at ~5s and the one after at ~10s — but the capture may end before the second one. Getting exactly 1 message is plausible and would cause a test failure despite the system working correctly. The comment is:
Evidence Summary 1. File Snapshot (HEAD)
2. File Snapshot (HEAD)
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Maintainability]
score_itfis only used by integration tests (the BUILD file setsdefault_testonly = True), but thebazel_depis declared as a non-dev dependency. This forces all downstream consumers ofscore_loggingto resolvescore_itf, even though they don't need it.Other test-only dependencies in this file (e.g.,
score_process,score_platform) usedev_dependency = True. Apply the same here:Context for Agents
Chain of Thought
score_loggingwill be forced to resolvescore_itfeven though it's only used by tests. This is a direct consequence of the missingdev_dependency = Trueflag.Reasoning
The comment is accurate and actionable:
New code introduced by this PR: The
bazel_dep(name = "score_itf", version = "0.1.0")on line 124 is entirely new — the base MODULE.bazel ends at line 122. This is not legacy debt.score_itfis only used in test code: The only reference toscore_itfis intests/integration/BUILD, which:package(default_testonly = True)on line 16load("@score_itf//:defs.bzl", "py_itf_test")and@score_itf//configscore_itfInconsistency with existing patterns: Other test-only dependencies in MODULE.bazel consistently use
dev_dependency = True:score_process(line 34):dev_dependency = Truescore_platform(line 35):dev_dependency = Truescore_toolchains_gcc(line 38):dev_dependency = Truescore_toolchains_qnx(line 39):dev_dependency = TrueReal downstream impact: In Bazel's module system, non-dev dependencies propagate to downstream consumers. Missing
dev_dependency = Truemeans any project depending onscore_loggingwill need to resolvescore_itf, even though they have no use for it.The fix is concrete and correct: Adding
dev_dependency = Trueis the right solution, consistent with the existing codebase conventions.Evidence Summary
1. File Snapshot (HEAD)
tests/integration/BUILD2. File Snapshot (HEAD)
MODULE.bazel3. File Snapshot (BASE)
MODULE.bazel115-1254. File Lookup
BUILD105. File Lookup
BUILD.bazel106. File Snapshot (HEAD)
examples/BUILD7. File Snapshot (HEAD)
score/datarouter/BUILD8. File Snapshot (HEAD)
BUILD