Integration tests consolidation #49
Conversation
Change debian12 to debian13, which will result in faster docker builds (new version does not require building gcc). Update version of io-module and cxxopts.
Add using test Context, which was created for this tests with test Packages and Apps. Fix some tests. Add setup at the beginning of tests, so each pytest call is consistent.
📝 WalkthroughWalkthroughReplaces per-session fixture with pytest_sessionstart/finish hooks, centralizes pre-test Packager/Docker image builds, migrates many test data files (deletions and ignore), updates Docker base images (debian12→debian13), adds CMCONF config/install into Dockerfiles, and adds nil-safety for internal build maps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Pytest as Pytest Runner
participant Session as pytest_sessionstart
participant ImgCheck as check_or_remove_images()
participant VCS as Git (test context)
participant Builder as Go build (Packager)
participant Packager as Packager binary
participant Docker as Docker Engine
participant Tests as Integration Tests
Note over Session,ImgCheck: Session setup (new flow)
Pytest->>Session: start
Session->>ImgCheck: verify or remove images (--remove_images?)
ImgCheck->>Docker: remove or validate images
Session->>VCS: init_context() (clone/pull test context)
VCS-->>Session: provide test_context & test_images
Session->>Builder: build Packager binary
Builder-->>Session: packager_binary ready
Session->>Packager: use packager to build images
Packager->>Docker: create images
Docker-->>Packager: images available
Pytest->>Tests: run (packager_binary, context, test_images)
Tests->>Packager: invoke run_packager(--use-local-repo?)
Packager->>Docker: perform build/run actions
Docker-->>Packager: results
Tests-->>Pytest: assertions/results
Pytest->>Session: finish -> pytest_sessionfinish -> clean()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration_tests/test_build_image.py (1)
63-73: Fix hardcoded image reference that doesn't exist.The test hardcodes
fedora43on line 63, but nofedora43Dockerfile exists in the test context. Available images are:fedora40,fedora41,ubuntu2404,debian13, and others. Either replacefedora43with an available image (e.g.,fedora40orfedora41) or ensure thefedora43image is added to the test context.
♻️ Duplicate comments (3)
example_context/docker/fedora40/Dockerfile (1)
23-26: Verify the multi-stage build configuration.Same issue as in
example_context/docker/fedora41/Dockerfile: thepackage-contextbuild stage is not defined in this Dockerfile. Please ensure the multi-stage build setup is correct.example_context/docker/ubuntu2404/Dockerfile (1)
23-26: Verify the multi-stage build configuration.Same issue as in other Dockerfiles in this PR: the
package-contextbuild stage referenced in theCOPY --from=package-contextinstruction is not visible in this file.example_context/docker/fleet-os-2/Dockerfile (1)
45-48: Verify the multi-stage build configuration.Same issue as in other Dockerfiles: the
package-contextbuild stage is not defined in this file. Please ensure the multi-stage build is properly configured.
🧹 Nitpick comments (6)
tests/test_utils/test_utils.py (1)
27-43: Address TODO: Remove hardcoded branch checkout.The init_context() function properly manages test context synchronization by cloning/pulling the remote repository. However, Line 38 contains a TODO to remove the hardcoded checkout of branch "BAF-1202/test-context".
This temporary branch checkout should be removed before merging to ensure tests run against the default branch. Would you like me to generate an issue to track this cleanup task?
Note: The function is called at module import time (line 43), which means it performs I/O operations during import. This is acceptable for test utilities but be aware of potential side effects.
example_context/docker/debian13/Dockerfile (1)
4-4: Hardcoded root password in test image.The static analysis tool flagged the use of
chpasswdwith a hardcoded password ("1234"). While this is acceptable for a test/development Docker image, ensure this image is never used in production environments.tests/integration_tests/test_build_package.py (1)
757-757: Minor grammar issues in docstrings.The docstrings use "an CMake" and "an Meson" which should be "a CMake" and "a Meson" respectively (since these words start with consonant sounds).
Also applies to: 773-773, 791-791, 809-809, 827-827, 845-845, 863-863
tests/conftest.py (3)
42-43: Remove unusedsessionparameter.The
sessionparameter inpytest_sessionfinishis unused. While this is a pytest hook signature, you can use an underscore prefix to indicate it's intentionally unused.-def pytest_sessionfinish(session): +def pytest_sessionfinish(session): # noqa: ARG001 clean()Or alternatively:
-def pytest_sessionfinish(session): +def pytest_sessionfinish(_session): clean()
73-83: Consider warning instead of exiting when images exist.The current behavior exits pytest if test images already exist without
--remove_images. This is strict but could be frustrating for users who may want to reuse existing images for faster iteration. Consider offering a--reuse_imagesoption or making the default behavior less strict.If the strict behavior is intentional for test consistency, consider improving the error message to explain why:
if len(existing_images) > 0: - pytest.exit(f"Images {existing_images} already exists. Use --remove_images to remove it.") + pytest.exit(f"Images {existing_images} already exist. Use --remove_images to remove and rebuild them, ensuring test consistency.")
64-64: Address or remove TODO comment.There's a TODO comment indicating this feature needs testing. Consider creating an issue to track this or remove the comment if it has been verified.
Would you like me to open an issue to track testing the
--remove_imagesfeature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (94)
.gitignore(1 hunks)README.md(1 hunks)example_context/app/io-module/io-module_debug.json(3 hunks)example_context/app/io-module/io-module_release.json(3 hunks)example_context/app/mission-module/mission-module_debug.json(1 hunks)example_context/app/mission-module/mission-module_release.json(1 hunks)example_context/config/CMCONF_FLEET_PROTOCOLConfig.cmake(1 hunks)example_context/config/README.md(1 hunks)example_context/docker/debian13/Dockerfile(2 hunks)example_context/docker/fedora40/Dockerfile(1 hunks)example_context/docker/fedora41/Dockerfile(1 hunks)example_context/docker/fleet-os-2/Dockerfile(1 hunks)example_context/docker/ubuntu2404/Dockerfile(1 hunks)example_context/package/ba-logger/ba-logger_debug.json(1 hunks)example_context/package/ba-logger/ba-logger_release.json(1 hunks)example_context/package/boost/boost_debug.json(1 hunks)example_context/package/boost/boost_release.json(1 hunks)example_context/package/cpprestsdk/cpprestsdk_debug.json(1 hunks)example_context/package/cpprestsdk/cpprestsdk_release.json(1 hunks)example_context/package/cxxopts/cxxopts_v3_debug.json(3 hunks)example_context/package/cxxopts/cxxopts_v3_release.json(3 hunks)example_context/package/fleet-http-client-shared/fleet_http_client_debug.json(1 hunks)example_context/package/fleet-http-client-shared/fleet_http_client_release.json(1 hunks)example_context/package/fleet-protocol-cpp/fleet_protocol_cpp_debug.json(1 hunks)example_context/package/fleet-protocol-cpp/fleet_protocol_cpp_release.json(1 hunks)example_context/package/fleet-protocol-interface/fleet_protocol_debug.json(1 hunks)example_context/package/fleet-protocol-interface/fleet_protocol_release.json(1 hunks)example_context/package/fleet-protocol-internal-client/internal_client_debug.json(1 hunks)example_context/package/fleet-protocol-internal-client/internal_client_release.json(1 hunks)example_context/package/nlohmann-json/nlohmann_json_debug.json(1 hunks)example_context/package/nlohmann-json/nlohmann_json_release.json(1 hunks)example_context/package/protobuf/protobuf_debug_v21.12.json(1 hunks)example_context/package/protobuf/protobuf_release_v21.12.json(1 hunks)example_context/package/spdlog/spdlog_debug.json(1 hunks)example_context/package/spdlog/spdlog_release.json(1 hunks)example_context/package/statesmurf/statesmurf_debug.json(1 hunks)example_context/package/statesmurf/statesmurf_release.json(1 hunks)example_context/package/zlib/zlib_debug.json(1 hunks)example_context/package/zlib/zlib_release.json(1 hunks)internal/docker/default_test.go(3 hunks)tests/README.md(1 hunks)tests/conftest.py(4 hunks)tests/integration_tests/test_build_apps.py(6 hunks)tests/integration_tests/test_build_image.py(2 hunks)tests/integration_tests/test_build_package.py(26 hunks)tests/integration_tests/test_create_sysroot.py(8 hunks)tests/integration_tests/test_invalid_arguments.py(3 hunks)tests/test_data/example/app/io-module/io-module_debug.json(0 hunks)tests/test_data/example/app/io-module/io-module_release.json(0 hunks)tests/test_data/example/app/mission-module/mission-module_debug.json(0 hunks)tests/test_data/example/app/mission-module/mission-module_release.json(0 hunks)tests/test_data/example/docker(0 hunks)tests/test_data/test_packages/test_package_1/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_06/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_06/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_17/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_18/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_21/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_21/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_22/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_22/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_23/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_23/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_1_25/zlib_debug.json(0 hunks)tests/test_data/test_packages/test_package_1_25/zlib_release.json(0 hunks)tests/test_data/test_packages/test_package_2/curl_debug.json(0 hunks)tests/test_data/test_packages/test_package_2/curl_release.json(0 hunks)tests/test_data/test_packages/test_package_2_06/curl_debug.json(0 hunks)tests/test_data/test_packages/test_package_2_06/curl_release.json(0 hunks)tests/test_data/test_packages/test_package_2_17/curl_release.json(0 hunks)tests/test_data/test_packages/test_package_2_22/curl_debug.json(0 hunks)tests/test_data/test_packages/test_package_2_22/curl_release.json(0 hunks)tests/test_data/test_packages/test_package_2_23/curl_debug.json(0 hunks)tests/test_data/test_packages/test_package_2_23/curl_release.json(0 hunks)tests/test_data/test_packages/test_package_3/lz4_debug.json(0 hunks)tests/test_data/test_packages/test_package_3/lz4_release.json(0 hunks)tests/test_data/test_packages/test_package_3_06/lz4_debug.json(0 hunks)tests/test_data/test_packages/test_package_3_06/lz4_release.json(0 hunks)tests/test_data/test_packages/test_package_4/bzip2_debug.json(0 hunks)tests/test_data/test_packages/test_package_4/bzip2_release.json(0 hunks)tests/test_data/test_packages/test_package_4_06/bzip2_debug.json(0 hunks)tests/test_data/test_packages/test_package_4_06/bzip2_release.json(0 hunks)tests/test_data/test_packages/test_package_5/nlohmann_json_debug.json(0 hunks)tests/test_data/test_packages/test_package_5/nlohmann_json_release.json(0 hunks)tests/test_data/test_packages/test_package_6/protozero_debug.json(0 hunks)tests/test_data/test_packages/test_package_6/protozero_release.json(0 hunks)tests/test_data/test_packages/test_package_7/modbuspp_debug.json(0 hunks)tests/test_data/test_packages/test_package_7/modbuspp_release.json(0 hunks)tests/test_data/test_packages/test_package_8/gtest_debug.json(0 hunks)tests/test_data/test_packages/test_package_8/gtest_release.json(0 hunks)tests/test_data/test_packages/test_package_9/spdlog_debug.json(0 hunks)tests/test_data/test_packages/test_package_9/spdlog_release.json(0 hunks)tests/test_utils/test_utils.py(3 hunks)
💤 Files with no reviewable changes (46)
- tests/test_data/test_packages/test_package_2_22/curl_debug.json
- tests/test_data/example/app/mission-module/mission-module_debug.json
- tests/test_data/test_packages/test_package_1_23/zlib_release.json
- tests/test_data/test_packages/test_package_8/gtest_release.json
- tests/test_data/test_packages/test_package_8/gtest_debug.json
- tests/test_data/example/docker
- tests/test_data/test_packages/test_package_4_06/bzip2_debug.json
- tests/test_data/test_packages/test_package_2_06/curl_release.json
- tests/test_data/test_packages/test_package_2/curl_debug.json
- tests/test_data/test_packages/test_package_3/lz4_release.json
- tests/test_data/test_packages/test_package_3_06/lz4_debug.json
- tests/test_data/test_packages/test_package_1_25/zlib_release.json
- tests/test_data/test_packages/test_package_3/lz4_debug.json
- tests/test_data/test_packages/test_package_2_17/curl_release.json
- tests/test_data/test_packages/test_package_2/curl_release.json
- tests/test_data/test_packages/test_package_2_23/curl_debug.json
- tests/test_data/test_packages/test_package_6/protozero_release.json
- tests/test_data/test_packages/test_package_1_25/zlib_debug.json
- tests/test_data/test_packages/test_package_2_06/curl_debug.json
- tests/test_data/test_packages/test_package_1_17/zlib_debug.json
- tests/test_data/test_packages/test_package_4/bzip2_release.json
- tests/test_data/example/app/mission-module/mission-module_release.json
- tests/test_data/test_packages/test_package_7/modbuspp_debug.json
- tests/test_data/test_packages/test_package_1/zlib_debug.json
- tests/test_data/test_packages/test_package_5/nlohmann_json_release.json
- tests/test_data/test_packages/test_package_4_06/bzip2_release.json
- tests/test_data/example/app/io-module/io-module_release.json
- tests/test_data/test_packages/test_package_1_23/zlib_debug.json
- tests/test_data/test_packages/test_package_3_06/lz4_release.json
- tests/test_data/test_packages/test_package_1_21/zlib_debug.json
- tests/test_data/test_packages/test_package_1_06/zlib_debug.json
- tests/test_data/test_packages/test_package_9/spdlog_release.json
- tests/test_data/test_packages/test_package_2_22/curl_release.json
- tests/test_data/test_packages/test_package_1_21/zlib_release.json
- tests/test_data/test_packages/test_package_5/nlohmann_json_debug.json
- tests/test_data/test_packages/test_package_1_06/zlib_release.json
- tests/test_data/test_packages/test_package_2_23/curl_release.json
- tests/test_data/test_packages/test_package_7/modbuspp_release.json
- tests/test_data/test_packages/test_package_1/zlib_release.json
- tests/test_data/test_packages/test_package_9/spdlog_debug.json
- tests/test_data/example/app/io-module/io-module_debug.json
- tests/test_data/test_packages/test_package_6/protozero_debug.json
- tests/test_data/test_packages/test_package_1_22/zlib_debug.json
- tests/test_data/test_packages/test_package_1_22/zlib_release.json
- tests/test_data/test_packages/test_package_4/bzip2_debug.json
- tests/test_data/test_packages/test_package_1_18/zlib_release.json
🧰 Additional context used
🧬 Code graph analysis (8)
internal/docker/default_test.go (3)
internal/prerequisites/initializer.go (1)
CreateAndInitialize(60-67)internal/docker/Docker.go (1)
Docker(19-31)internal/constants/Constants.go (1)
DefaultSSHPort(12-12)
tests/integration_tests/test_build_image.py (1)
tests/test_utils/test_utils.py (1)
does_image_exist(76-83)
tests/test_utils/test_utils.py (1)
tests/test_utils/common.py (2)
PackagerExpectedResult(20-29)PackagerReturnCode(4-17)
tests/integration_tests/test_build_package.py (4)
tests/test_utils/test_utils.py (4)
prepare_packages(101-108)does_package_support_image(187-204)is_tracked(111-149)run_packager(215-323)tests/conftest.py (4)
test_image(87-94)test_repo(103-104)packager_binary(113-116)context(98-99)tests/test_utils/common.py (2)
PackagerExpectedResult(20-29)PackagerReturnCode(4-17)internal/packager_error/PackagerError.go (1)
CONTEXT_ERROR(10-10)
tests/integration_tests/test_invalid_arguments.py (1)
tests/conftest.py (4)
packager_binary(113-116)context(98-99)test_image(87-94)test_repo(103-104)
tests/integration_tests/test_create_sysroot.py (1)
tests/test_utils/test_utils.py (3)
prepare_packages(101-108)run_packager(215-323)is_tracked(111-149)
tests/conftest.py (2)
tests/test_utils/test_utils.py (4)
init_test_repo(46-52)clean(68-73)init_test_sysroot(55-60)run_packager(215-323)tests/test_utils/common.py (1)
PackagerExpectedResult(20-29)
tests/integration_tests/test_build_apps.py (4)
tests/test_utils/test_utils.py (1)
prepare_packages(101-108)tests/test_utils/common.py (2)
PackagerExpectedResult(20-29)PackagerReturnCode(4-17)tests/conftest.py (4)
test_image(87-94)packager_binary(113-116)context(98-99)test_repo(103-104)internal/packager_error/PackagerError.go (1)
DEFAULT_ERROR(8-8)
🪛 Checkov (3.2.334)
example_context/docker/debian13/Dockerfile
[medium] 4-4: Ensure that 'chpasswd' is not used to set or remove passwords
(CKV2_DOCKER_17)
🪛 Ruff (0.14.8)
tests/test_utils/test_utils.py
301-301: subprocess call: check for execution of untrusted input
(S603)
tests/conftest.py
17-17: Starting a process with a partial executable path
(S607)
22-22: subprocess call: check for execution of untrusted input
(S603)
23-23: Starting a process with a partial executable path
(S607)
42-42: Unused function argument: session
(ARG001)
🔇 Additional comments (54)
example_context/package/fleet-http-client-shared/fleet_http_client_release.json (1)
38-38: Update to Debian 13 looks good.Debian 13 is the current stable production release, initially released on August 9th, 2025, and this upgrade aligns well with the PR's infrastructure modernization goals.
example_context/package/spdlog/spdlog_debug.json (1)
28-34: Base image upgrade is straightforward; verify consistency across codebase.The
debian13image name update aligns with the broader Docker base image upgrade. Ensure this change is applied consistently across all package contexts.example_context/package/spdlog/spdlog_release.json (1)
28-34: Consistent with debug variant upgrade.Aligned with spdlog_debug.json and broader Debian 13 migration.
example_context/package/nlohmann-json/nlohmann_json_release.json (1)
27-34: Base image upgrade aligned with project-wide Debian 13 migration.example_context/app/mission-module/mission-module_debug.json (1)
25-33: Base image upgrade consistent across apps and packages.example_context/package/ba-logger/ba-logger_release.json (1)
30-37: Base image upgrade maintains dependency consistency.ba-logger's dependency (spdlog) has also been upgraded to debian13, ensuring build compatibility.
example_context/app/mission-module/mission-module_release.json (1)
25-33: Consistent with mission-module debug variant upgrade.example_context/package/zlib/zlib_release.json (1)
24-33: Base image upgrade for core dependency zlib.example_context/package/cxxopts/cxxopts_v3_release.json (2)
27-34: Base image upgrade aligned with Debian 13 migration.
4-7: The cxxopts version bump from v3.0.5 to v3.1.0 is valid. The v3.1.0 tag exists in the upstream repository and is an official release.example_context/package/fleet-protocol-interface/fleet_protocol_release.json (1)
31-31: Configuration updates align with infrastructure modernization.The Debian base image update and addition of new image variants extend the build matrix support. These metadata changes are non-breaking and consistent with the PR's modernization goals.
Also applies to: 35-36
example_context/package/statesmurf/statesmurf_debug.json (1)
33-33: Base image update is consistent.Debian 12 → 13 upgrade aligns with infrastructure modernization across the PR.
README.md (1)
73-73: Documentation updated to reflect image name changes.All three example commands correctly reference the updated
debian13image name. The changes maintain consistency with the infrastructure updates across the PR.Also applies to: 79-79, 85-85
example_context/package/boost/boost_release.json (1)
30-30: Configuration updated consistently with project-wide changes.Debian base image reference updated to match infrastructure modernization.
example_context/package/fleet-protocol-internal-client/internal_client_release.json (1)
34-34: Base image updated consistently.Debian 12 → 13 upgrade aligns with project-wide infrastructure changes.
example_context/app/io-module/io-module_release.json (1)
5-5: Version and base image updates are coordinated.The io-module revision and package version were both incremented to v1.3.3, aligning with integration of upstream PRs #47 and #48 as described in the PR objectives.
Also applies to: 17-17, 28-28
example_context/package/boost/boost_debug.json (1)
30-30: Configuration updated consistently.Debian base image updated to align with infrastructure modernization.
example_context/package/protobuf/protobuf_release_v21.12.json (1)
30-30: Configuration updated consistently with infrastructure changes.Debian base image reference modernized to align with project-wide updates.
.gitignore (1)
16-16: Approved.The addition of
tests/test_data/test_contextfollows the existing pattern for test data directories and aligns with the test consolidation objectives.example_context/app/io-module/io-module_debug.json (1)
5-5: Approved.Version and Docker image updates are consistent across Git.Revision, Package.VersionTag, and DockerMatrix.ImageNames. The patch version bump (v1.3.2→v1.3.3) and debian12→debian13 upgrade align with the PR's infrastructure modernization.
Verify that the Debian 13 Docker image is built and available. Please confirm the image is referenced in a Dockerfile or build configuration included in this PR.
Also applies to: 17-17, 28-28
example_context/package/cpprestsdk/cpprestsdk_debug.json (1)
33-33: Approved.Docker image update to debian13 is consistent with the broader modernization effort in the PR. The package version (v2.10.20) and dependencies remain unchanged.
example_context/package/cpprestsdk/cpprestsdk_release.json (1)
33-33: Approved.Docker image update mirrors the debug variant, maintaining consistency across release and debug configurations.
example_context/package/fleet-http-client-shared/fleet_http_client_debug.json (1)
38-38: Approved.Docker image update to debian13 is consistent with peer packages. Dependencies and build configuration remain unchanged.
example_context/package/fleet-protocol-interface/fleet_protocol_debug.json (1)
31-31: Approved with verification request for fleet-os-2 image consistency.The debian12→debian13 update is consistent. However, the addition of "fleet-os-2" image is selective (present here but absent from earlier reviewed files like cpprestsdk and fleet-http-client-shared). Ensure this pattern is intentional and consistently applied to all packages that need it.
Verify that "fleet-os-2" image additions are applied consistently across all package configurations that should support it. Check if this image was added to other files not provided in this review.
Also applies to: 36-36
tests/README.md (1)
38-39: Verify fedora43 stability and intentional version jump.The default test image was changed from fedora40 to fedora43, skipping fedora41 and fedora42. While the documentation is clear, this significant jump warrants verification:
- Is fedora43 tested and stable for this project?
- Were fedora41/42 intentionally skipped or are they also supported but not mentioned?
- Has the test suite been validated against fedora43?
Additionally, verify that the
--remove_imagesflag is properly implemented in the test configuration (conftest.py).example_context/package/protobuf/protobuf_debug_v21.12.json (1)
30-36: Approved.Docker image matrix updated consistently: debian12→debian13 upgrade applied, and fleet-os-2 image included (consistent with foundational packages like fleet-protocol-interface). Build configuration unchanged.
internal/docker/default_test.go (1)
13-13: LGTM! Test data updated consistently.All three test functions have been updated to initialize Docker instances with debian13 instead of debian12. The test logic remains unchanged, only the base image reference has been updated to align with the infrastructure modernization.
Also applies to: 35-35, 61-61
example_context/package/statesmurf/statesmurf_release.json (1)
33-33: LGTM! Docker base image updated.The update from debian12 to debian13 is consistent with the infrastructure modernization effort across the PR.
example_context/package/ba-logger/ba-logger_debug.json (1)
32-32: LGTM! Docker base image updated.The update from debian12 to debian13 aligns with the coordinated infrastructure upgrade across the codebase.
example_context/package/cxxopts/cxxopts_v3_debug.json (2)
30-30: LGTM! Docker base image updated.The Docker image upgrade from debian12 to debian13 is consistent with the infrastructure modernization across the PR.
6-6: No action required. cxxopts v3.1.0 is a backward-compatible minor release with no breaking changes from v3.0.5. The upgrade introduces new features (multiple long names, program() method, iterator support) and bug fixes without affecting existing code that depends on this library.tests/test_utils/test_utils.py (4)
10-24: LGTM! Test context centralized.The introduction of TEST_CONTEXT_URL and TEST_CONTEXT_PATH constants along with the updated test_config structure provides better organization for test data management. The test_images list will be populated by init_context().
228-228: LGTM! Enhanced test flexibility.The new use_local_repo and show_output parameters provide better control over packager execution behavior. The default values (False and True respectively) maintain backward compatibility with existing test code.
Also applies to: 233-233
295-296: LGTM! Flag handling implemented correctly.The use_local_repo parameter is properly translated to the --use-local-repo command-line flag, following the same pattern as other boolean flags in the function.
298-315: LGTM! Output control and code formatting improved.The conditional output handling based on show_output provides better test log management. The Popen invocation has been reformatted with explicit parameter names for improved readability.
Re: Static analysis warning (S603): The Ruff warning about untrusted subprocess input is a false positive in this test utility context. The parameters are constructed from controlled test inputs and validated by the function's precondition checks (lines 237-248).
example_context/package/fleet-protocol-cpp/fleet_protocol_cpp_release.json (1)
33-37: LGTM! Docker matrix updated.Two improvements to the Docker matrix:
- Base image upgraded from debian12 to debian13
- Added fleet-os-2 to expand platform support
Both changes align with the infrastructure modernization and test consolidation objectives of this PR.
example_context/package/fleet-protocol-cpp/fleet_protocol_cpp_debug.json (1)
33-37: LGTM! Debug configuration matches release.The Docker matrix updates mirror those in the release configuration:
- debian12 → debian13 upgrade
- fleet-os-2 addition
This maintains consistency between debug and release build environments.
example_context/package/nlohmann-json/nlohmann_json_debug.json (1)
29-29: Approved. The debian13 Docker image is properly configured in the repository atexample_context/docker/debian13/Dockerfile, confirming the base image upgrade is supported.tests/integration_tests/test_invalid_arguments.py (1)
30-43: LGTM! Context fixture integration is clean.The addition of the
contextfixture parameter to these three test functions is consistent with the broader test infrastructure refactoring. The tests now properly rely on the centralized context fixture instead of inline initialization, improving maintainability.Also applies to: 46-61, 63-77
example_context/config/README.md (1)
1-17: LGTM! Clear documentation.The documentation provides a clear explanation of the FLEET_PROTOCOL CMCONF system, its purpose, and key configuration variables. The references to external resources are helpful for users.
example_context/config/CMCONF_FLEET_PROTOCOLConfig.cmake (2)
1-20: LGTM! CMCONF initialization is correct.The CMCONF system initialization and basic configuration settings are properly structured. The default values for local usage, paths, and authorization are appropriate.
22-26: The hard-coded URLhttps://gitea.bringauto.com/fleet-protocol/package-repositoryis publicly accessible and returns HTTP 200 responses. Both the base server and the specific repository path are reachable without authentication issues. No action is required.example_context/docker/debian13/Dockerfile (1)
22-28: LGTM!The CMCONF integration pattern is consistent with the other Dockerfiles in the project. The multi-stage build approach using
package-contextfor copying configuration files is appropriate.tests/integration_tests/test_build_package.py (2)
737-754: LGTM!The Meson build test correctly checks for image support and follows the established test pattern.
14-14: LGTM!The package naming convention has been consistently updated across all tests to use the new
test-package-<suffix>format, which aligns with the PR objective of using a test Context with simple Packages.Also applies to: 34-35, 74-75, 100-100, 124-125, 162-165, 203-203, 234-234, 256-256, 280-280, 302-302, 326-326, 362-362, 400-404, 431-435, 476-480, 516-516, 538-539, 562-562, 590-590, 616-616, 637-638, 664-665, 689-689, 723-723
tests/integration_tests/test_build_apps.py (3)
177-182: LGTM!Good addition of
SO_REUSEADDRbefore binding. This prevents "Address already in use" errors when tests are re-run quickly after the socket was closed.
109-132: LGTM!The new test for building apps with dependencies correctly integrates with the helper function and uses
use_local_repo=Trueto access the locally built packages.
17-17: LGTM!App naming convention consistently updated to use
test-app-*format across all tests.Also applies to: 44-44, 137-137, 171-171
tests/integration_tests/test_create_sysroot.py (3)
17-17: LGTM!Package names consistently updated to use the new
test-package-*naming convention across all sysroot tests.Also applies to: 51-51, 106-106, 146-146, 173-173
59-75: LGTM!Image names and OS paths have been correctly updated to reflect the new test matrix (ubuntu2404, debian13, fedora43) with corresponding path mappings.
Also applies to: 94-94, 114-130, 154-164
201-206: LGTM!The sysroot artifact expectations have been appropriately updated to match the new test package outputs (shared libraries and headers for test-package-a and test-package-b).
tests/conftest.py (2)
8-39: LGTM!The
pytest_sessionstarthook properly orchestrates the test setup phase: checking/removing Docker images, building the Packager binary, and building Docker images. The separation of concerns and clear print statements improve debugging visibility.
97-99: LGTM!The fixtures have been appropriately simplified to return values from
test_config, with the build logic moved to the session start hook.Also applies to: 112-116
example_context/package/zlib/zlib_debug.json (1)
27-27: Docker base image correctly upgraded to debian13.The change updates the Docker matrix to use
debian13instead ofdebian12, which aligns with infrastructure modernization objectives. The JSON syntax is valid, and the update is consistently applied across related zlib configuration files (bothzlib_debug.jsonandzlib_release.json).
example_context/package/fleet-protocol-internal-client/internal_client_debug.json
Show resolved
Hide resolved
That1trigga21
left a comment
That1trigga21
left a comment
There was a problem hiding this comment.
Absolutely phenomenal!!! Thank you.
koudis
left a comment
koudis
left a comment
There was a problem hiding this comment.
It looks nice. No comments here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_utils/test_utils.py (1)
27-39: Add error handling and validate assumptions about directory contents.The function has several concerns:
Missing error handling: Git operations (clone/pull) can fail due to network issues, authentication, repository corruption, or permissions. These failures will crash the module import (since
init_context()is called at line 41).Unsafe assumption (Line 38): The code assumes
test_dockerscontains only directories. If it contains regular files, they'll be incorrectly added totest_images.Misleading docstring: The docstring says "Update the context" but the function also initializes (clones) the repository on first run.
Mixed responsibilities: The function both syncs the git repository and populates
test_images. Consider splitting these concerns.🔎 Proposed improvements
def init_context(): - """Update the context with the latest changes from the repository.""" + """Initialize or update the test context from the remote repository.""" + try: - os.makedirs(test_config["test_context"], exist_ok=True) + os.makedirs(test_config["test_context"], exist_ok=True) - if os.path.exists(test_config["test_context"] + "/.git"): - test_context = git.Repo(test_config["test_context"]) - test_context.git.pull() - else: - test_context = git.Repo.clone_from( - TEST_CONTEXT_URL, test_config["test_context"] - ) + if os.path.exists(test_config["test_context"] + "/.git"): + test_context = git.Repo(test_config["test_context"]) + test_context.git.pull() + else: + test_context = git.Repo.clone_from( + TEST_CONTEXT_URL, test_config["test_context"] + ) + except (git.exc.GitCommandError, git.exc.GitError) as e: + raise RuntimeError(f"Failed to initialize test context: {e}") from e - test_config["test_images"] = sorted(os.listdir(test_config["test_dockers"])) + # Only include directories, not files + test_config["test_images"] = sorted([ + item for item in os.listdir(test_config["test_dockers"]) + if os.path.isdir(os.path.join(test_config["test_dockers"], item)) + ])
🧹 Nitpick comments (2)
tests/test_utils/test_utils.py (2)
10-11: Verify the test-context repository branch before merge.The PR objectives mention "TODO: change test-context branch before merge." Ensure the correct branch is being used or specify the branch explicitly in the URL if needed.
Additionally, consider making the URL configurable via an environment variable to support alternative test contexts in different environments (e.g., forks, local mirrors, offline testing).
226-314: LGTM: Parameter additions and output handling are well-implemented.The addition of
use_local_repoandshow_outputparameters is well-designed:
- Both have sensible defaults
use_local_repoenables testing with local repositoriesshow_outputprovides control over verbosity for cleaner test output- Implementation follows existing patterns in the function
Minor note: The ANSI color codes (lines 296-297) may not render correctly in all environments (e.g., Windows cmd without ANSI support, non-TTY output). This is acceptable for test utilities but could be enhanced with terminal capability detection if needed.
TODO:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.