[DEMO — do not merge] Green fprime-examples integration tests (full stack)#198
[DEMO — do not merge] Green fprime-examples integration tests (full stack)#198devin-ai-integration[bot] wants to merge 66 commits into
Conversation
…k the container size
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
…s the re-serialization logic
* fix: reject invalid SpacePacketDeframer control fields * fix: reject all invalid deframer control fields * fix: narrow deframer control-field validation * fix: narrow fprime deframer validation to PVN * docs: document SpacePacketDeframer PVN validation * style: format SpacePacketDeframer test
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Original prompt from thomas.boyer.chammard
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Coverage report — base
|
| Module | Line | Function | Branch |
|---|---|---|---|
CFDP/Checksum |
71.15 | 57.14 | 53.85 |
Drv/AsyncByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/ByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/Ip |
45.12 | 57.38 | 27.17 |
Drv/TcpClient |
75.00 | 100.00 | 47.37 |
Drv/TcpServer |
84.72 | 100.00 | 60.00 |
Drv/Udp |
68.09 | 90.91 | 42.86 |
Fw/Buffer |
81.25 | 89.47 | 58.62 |
Fw/DataStructures |
98.21 | 97.14 | 82.84 |
Fw/Dp |
94.83 | 96.67 | 95.95 |
Fw/FilePacket |
75.24 | 89.06 | 54.30 |
Fw/Log |
86.96 | 100.00 | 68.18 |
Fw/Logger |
100.00 | 100.00 | 100.00 |
Fw/SerializableFile |
90.00 | 100.00 | 79.41 |
Fw/Time |
88.62 | 85.48 | 86.84 |
Fw/Tlm |
63.77 | 72.73 | 44.12 |
Fw/Types |
55.04 | 58.63 | 35.70 |
Os |
17.98 | 19.40 | 14.47 |
Os/Generic |
89.10 | 96.30 | 67.18 |
Os/Generic/Types |
92.45 | 91.67 | 82.14 |
Os/Posix |
62.00 | 84.21 | 43.87 |
Svc/ActiveRateGroup |
100.00 | 100.00 | 92.31 |
Svc/ActiveTextLogger |
79.05 | 90.00 | 71.23 |
Svc/AssertFatalAdapter |
94.74 | 100.00 | 86.67 |
Svc/BufferAccumulator |
88.16 | 94.12 | 74.75 |
Svc/BufferLogger |
92.76 | 86.96 | 81.32 |
Svc/BufferManager |
99.05 | 100.00 | 83.64 |
Svc/BufferRepeater |
91.67 | 100.00 | 75.00 |
Svc/ChronoTime |
100.00 | 100.00 | 100.00 |
Svc/CmdDispatcher |
96.97 | 91.67 | 91.75 |
Svc/CmdSequencer |
93.89 | 97.12 | 84.57 |
Svc/CmdSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComLogger |
97.37 | 91.67 | 83.91 |
Svc/ComSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComStub |
98.51 | 100.00 | 85.19 |
Svc/DpCatalog |
77.56 | 100.00 | 65.80 |
Svc/DpManager |
97.44 | 100.00 | 100.00 |
Svc/DpWriter |
97.58 | 90.00 | 97.22 |
Svc/FileDownlink |
84.15 | 90.91 | 71.90 |
Svc/FileManager |
88.41 | 93.33 | 82.44 |
Svc/FileUplink |
92.35 | 96.55 | 80.95 |
Svc/FileWorker |
90.29 | 100.00 | 84.87 |
Svc/FprimeDeframer |
100.00 | 100.00 | 97.92 |
Svc/FprimeFramer |
100.00 | 100.00 | 95.45 |
Svc/FprimeRouter |
88.89 | 100.00 | 78.26 |
Svc/FpySequencer |
86.76 | 99.04 | 76.85 |
Svc/GenericHub |
100.00 | 100.00 | 85.56 |
Svc/Health |
100.00 | 100.00 | 88.66 |
Svc/LinuxTimer |
97.06 | 100.00 | 82.35 |
Svc/OsTime |
70.00 | 83.33 | 60.98 |
Svc/PassiveRateGroup |
100.00 | 100.00 | 89.47 |
Svc/PolyDb |
100.00 | 100.00 | 53.57 |
Svc/PosixTime |
100.00 | 100.00 | 75.00 |
Svc/PrmDb |
92.75 | 89.47 | 88.11 |
Svc/RateGroupDriver |
100.00 | 100.00 | 68.75 |
Svc/SeqDispatcher |
73.08 | 80.00 | 71.05 |
Svc/StaticMemory |
100.00 | 100.00 | 100.00 |
Svc/SystemResources |
98.63 | 100.00 | 76.19 |
Svc/TlmChan |
77.59 | 85.71 | 63.03 |
Svc/TlmPacketizer |
92.18 | 100.00 | 77.08 |
Svc/Version |
96.10 | 100.00 | 86.96 |
Utils |
20.04 | 26.44 | 24.07 |
Utils/Types |
92.11 | 95.83 | 77.08 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| Fw::Buffer data_buffer(fwBuffer.getData() + Fw::DpContainer::DATA_OFFSET, | ||
| fwBuffer.getSize() - Fw::DpContainer::MIN_PACKET_SIZE); |
There was a problem hiding this comment.
🔴 Compression processes uninitialized memory beyond actual data product payload
The working buffer for compression is sized to the full buffer capacity rather than the actual data content (fwBuffer.getSize() - MIN_PACKET_SIZE at Svc/DpCompressProc/DpCompressProc.cpp:74), so when the allocated buffer is larger than the payload, the compression loop iterates over uninitialized bytes.
Impact: Compressed data products may contain garbage data appended to the real payload, silently corrupting the output.
Buffer sizing mismatch between capacity and actual data
At Svc/DpCompressProc/DpCompressProc.cpp:56-57, the container header is deserialized, making container.getDataSize() available — this is the actual number of valid data bytes. However, at line 73-74, data_buffer is constructed with size fwBuffer.getSize() - Fw::DpContainer::MIN_PACKET_SIZE, which is the maximum data capacity of the buffer, not the actual data size.
In production, Svc::BufferManager allocates from fixed-size bins, so the returned buffer is often larger than requested. This means fwBuffer.getSize() - MIN_PACKET_SIZE can be significantly larger than container.getDataSize().
The compression loop at line 115 uses data_deser.getDeserializeSizeLeft() which starts at data_buffer.getSize() (the capacity), not container.getDataSize(). This causes the loop to process extra bytes beyond the valid data.
The unit tests at Svc/DpCompressProc/test/ut/AbstractState.cpp:22 happen to allocate exactly-sized buffers (chunk_size * chunks.size() + MIN_PACKET_SIZE), so the capacity equals the data size and the bug is masked.
The fix should use container.getDataSize() instead of fwBuffer.getSize() - Fw::DpContainer::MIN_PACKET_SIZE for the data buffer size.
| Fw::Buffer data_buffer(fwBuffer.getData() + Fw::DpContainer::DATA_OFFSET, | |
| fwBuffer.getSize() - Fw::DpContainer::MIN_PACKET_SIZE); | |
| Fw::Buffer data_buffer(fwBuffer.getData() + Fw::DpContainer::DATA_OFFSET, | |
| container.getDataSize()); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This code belongs to nasa#5235 (Svc/DpCompressProc), which is merged into this demo branch only so ExamplesDeployment builds — it's not part of this CI change, so I'm not modifying it here. That said, the finding looks legitimate (compressing fwBuffer.getSize() - MIN_PACKET_SIZE capacity rather than container.getDataSize() could process bytes past the payload); worth raising on nasa#5235 itself. cc @thomas-bc
| enum ProcType: U8 { | ||
| @ No Processing | ||
| PROC_TYPE_NONE = 0x00 | ||
| @ Processing type 0 | ||
| PROC_TYPE_ZERO = 0x01 | ||
| PROC_TYPE_ZLIB_DEFLATE = 0x01 | ||
| @ Processing type 1 | ||
| PROC_TYPE_ONE = 0x02 | ||
| @ Processing type 2 |
There was a problem hiding this comment.
🔍 Renaming ProcType enum value is a breaking API change
The enum value PROC_TYPE_ZERO = 0x01 was renamed to PROC_TYPE_ZLIB_DEFLATE = 0x01 and a new PROC_TYPE_NONE = 0x00 was added at default/config/DpCfg.fpp:15-24. While the numeric values are preserved (0x01 stays 0x01), this is a source-level breaking change: any downstream code referencing Fw::DpCfg::ProcType::PROC_TYPE_ZERO will fail to compile. Additionally, adding PROC_TYPE_NONE increases NUM_CONSTANTS from 3 to 4, which affects test code in Svc/DpWriter/test/ut/AbstractState.cpp:28 and Svc/DpWriter/test/ut/DpWriterTester.cpp:85 that iterates over NUM_CONSTANTS to generate random bitmasks. The production DpWriter code at Svc/DpWriter/DpWriter.cpp:168 uses NUM_PROCBUFFERSENDOUT_OUTPUT_PORTS instead, so production behavior is unaffected, but the DpWriter tests may now generate bitmask values that set bit 3 (corresponding to PROC_TYPE_NONE's position in the enum), which doesn't correspond to any output port.
(Refers to lines 15-24)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is nasa#5235's default/config/DpCfg.fpp, merged in only for the demo build — out of scope for this CI change, so no edits here. The NUM_CONSTANTS 3→4 knock-on for Svc/DpWriter tests is a valid thing to check on nasa#5235 upstream. cc @thomas-bc
| run_integration_tests: | ||
| description: "Run an additional job to build the deployment and run integration tests against it." | ||
| required: false | ||
| type: boolean | ||
| default: false | ||
| integration_gds_working_directory: | ||
| description: "Directory to start fprime-gds from (relative to repo root). Required when run_integration_tests is true." | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| integration_test_working_directory: | ||
| description: "Directory to run pytest from (relative to repo root). Required when run_integration_tests is true." | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| integration_binary: | ||
| description: "Path to the FSW binary to run (relative to integration_gds_working_directory). Required when run_integration_tests is true." | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| integration_binary_args: | ||
| description: "Arguments passed to the FSW binary." | ||
| required: false | ||
| type: string | ||
| default: "-a 127.0.0.1 -p 50000" | ||
| integration_gds_args: | ||
| description: "Extra arguments passed to fprime-gds (e.g. --dictionary path)." | ||
| required: false | ||
| type: string | ||
| default: "" | ||
| integration_pytest_args: | ||
| description: "Extra arguments passed to pytest (e.g. test paths, --dictionary path)." | ||
| required: false | ||
| type: string | ||
| default: "" |
There was a problem hiding this comment.
🔍 Integration test job definition appears missing from reusable workflow
The reusable workflow at .github/workflows/reusable-project-builder.yml adds input parameters for integration tests (run_integration_tests, integration_gds_working_directory, integration_binary, etc. at lines 22-56), and the examples workflow at .github/workflows/ext-build-examples-repo.yml:45 sets run_integration_tests: true. However, the diff for reusable-project-builder.yml only shows the build and runUT jobs — there is no runIntegrationTests job defined that would use these new inputs. If this job is truly missing, the integration test inputs would be accepted but silently ignored, and no integration tests would actually run.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
False positive — the runIntegration job is defined in reusable-project-builder.yml (below the build/runUT jobs); the review diff hunk here only shows the new inputs block. Proof it runs: this PR's run / Integration Tests job executed and reported 5 passed in 41.20s. The job is gated on if: ${{ inputs.run_integration_tests }} (default false), so other callers are unaffected.
| # FORK-DEMO OVERRIDE: point at JPL-Devin/fprime-examples PR #1 so the new | ||
| # DataProduct/test/int compression test is present. Revert to | ||
| # `nasa/fprime-examples` + `needs.get-branch.outputs.target-branch` before | ||
| # opening this upstream. | ||
| target_repository: JPL-Devin/fprime-examples | ||
| build_location: FlightExamples | ||
| run_unit_tests: true | ||
| target_ref: ${{ needs.get-branch.outputs.target-branch }} | ||
| run_integration_tests: true | ||
| integration_gds_working_directory: FlightExamples/ExamplesDeployment | ||
| integration_test_working_directory: FlightExamples | ||
| integration_binary: ../build-artifacts/*/ExamplesDeployment/bin/ExamplesDeployment | ||
| integration_gds_args: "--dictionary ../build-artifacts/*/ExamplesDeployment/dict/ExamplesDeploymentTopologyDictionary.json" | ||
| integration_pytest_args: "DataProduct/test/int ManagerWorker/Subtopology/test/int --dictionary build-artifacts/*/ExamplesDeployment/dict/ExamplesDeploymentTopologyDictionary.json --no-zmq" | ||
| target_ref: devin/1783371774-dp-compression-example |
There was a problem hiding this comment.
🔍 CI workflow hardcoded to fork branch — must be reverted before upstream merge
The examples workflow at .github/workflows/ext-build-examples-repo.yml:38-51 is hardcoded to point at JPL-Devin/fprime-examples with branch devin/1783371774-dp-compression-example instead of the standard nasa/fprime-examples with the dynamic branch from get-branch. The comments at lines 38-41 explicitly acknowledge this is a temporary override for demo purposes. This must be reverted before merging upstream, as it would cause CI to permanently depend on a fork branch that may be deleted.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct and intentional — this is the throwaway demo branch, and the fork-branch override is deliberate (see the comment right above it) so the not-yet-upstream DataProduct/test/int test is present. The real CI PR (#197) keeps target_repository: nasa/fprime-examples + needs.get-branch.outputs.target-branch. This demo branch is marked do-not-merge.
5 participants
Change Description
Demonstration only — do not merge. This branch stacks the full real-world dependency chain so the fprime-examples integration-test job can actually run green end-to-end:
runIntegrationjob inreusable-project-builder.yml+ext-build-examples-repo.ymlwiring).ext-build-examples-repo.yml:target_repository: JPL-Devin/fprime-examples,target_ref: devin/1783371774-dp-compression-example(fprime-examples PR Clean up deprecated serialization API across F Prime codebase #1) so the newDataProduct/test/int/test_dp_compression.pyis present.DpCompressionsubtopology +DpZLibCompressor), which the DataProduct test depends on and which is not yet ondevel.Rationale
Plain #197 red-only because (a)
nasa/fprime-examples@develdoesn't yet haveDataProduct/test/int, and (b) the overlaid F´ doesn't yet have theDpCompressionsubtopology (nasa#5235 is unmerged). This branch supplies both so you can watch the integration job go fully green.Testing/Review Recommendations
Watch the
run / Integration Testsjob: it builds ExamplesDeployment against overlaid in-dev F´ (+ nasa#5235), upgrades to in-devfprime-gds, starts GDS with decaf framing + FSW, then runspytest DataProduct/test/int ManagerWorker/Subtopology/test/int(DataProduct compression + 4 ManagerWorker).Future Work
Real upstream ordering: merge nasa#5235 → land the DataProduct test on
nasa/fprime-examples/devel→ merge the #197 CI change (kept clean, targetingnasa/fprime-examples). This demo branch is not for merge.AI Usage (see policy)
Demonstration branch assembled to validate the CI integration-test wiring end-to-end.
IAMAI
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/51e2b5a0a13340d6b2db328c7c6c36e8