Remove unused Os::File::m_path member - #149
Conversation
* Add F Prime unit testing skill for Devin Add a Devin skill (.devin/skills/fprime-unit-testing/SKILL.md) that provides step-by-step instructions for writing F Prime component unit tests. Covers: - Scaffold generation via fprime-util impl --ut - Tester / TestMain / GTestBase pattern - Helper function design for clean, readable tests - Rules-based testing with STest (AbstractState, TestState, Rules, Scenarios) - CMakeLists.txt registration with UT_AUTO_HELPERS - Quality checklist for coverage completeness Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Exclude .devin/ directory from spell checking Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Move UT skill to standard location; add AI instructions; trim content - Move from .devin/skills/ to .github/agents/_shared/skills/fprime-unit-testing.skill.md - Add unit-testing section to copilot-instructions.md so other AIs discover the skill - Trim skill from 441 to ~300 lines while preserving all sections - Exclude .github/agents/ from spell-check Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Rewrite §5 rules-based section from official docs Replace hallucinated macro patterns (TEST_STATE_DEF_RULE, RULES_DEF_RULE, AbstractState, etc.) with the official approach: - FW_RBT_DEFINE_RULE macro from TestUtils/RuleBasedTesting.hpp - Shadow state as Tester member (not separate AbstractState/TestState) - fprime-util new --rule-based-test scaffolding - Link to docs/how-to/rule-based-testing.md Based on the official guide at nasa/fprime/docs/how-to/rule-based-testing.md and the Svc/Ccsds/ApidManager exemplar. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Condense §5 rules-based section to defer to official docs Replace detailed scaffold/file-layout/shadow-state/rules/scenarios code examples with a brief orientation (decision table, core constructs, build note) and a pointer to docs/how-to/rule-based-testing.md. Skills should not duplicate canonical documentation — they provide the 'when/why' and defer the 'how' to the docs. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Add 'prefer linking existing docs' guidance to skill authoring skill New §4 instructs skill authors to search for pre-existing docs and link to them rather than duplicating procedural content. Skills should provide when/why + brief orientation + link; the canonical doc owns the full how-to. Duplicated content drifts and risks hallucination. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Update unit testing guide, trim skill to defer to it Expand docs/user-manual/overview/unit-testing.md with sections that were missing: scaffold generation (fprime-util impl --ut), Tester class structure, TestMain with COMMENT/REQUIREMENT macros, helper function patterns, CMakeLists.txt registration, clearHistory/ doDispatch patterns, port invocation, and a link to the rules-based testing guide. Fix escaped underscores in code blocks and add cpp language specifiers. Trim the skill from 242 to 108 lines — it now provides workflow order, key patterns to enforce, the rules-based decision table, and the quality checklist, deferring all implementation detail to the guide. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov> * Revise unit testing documentation for clarity Updated the documentation to reflect changes in file naming conventions and clarify the generation of test framework classes. --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: michael.d.starch <michael.d.starch@jpl.nasa.gov> Co-authored-by: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com>
Add '.github/actions/spelling/**' and '.github/agents/**' to the paths-ignore filter, matching the other CI workflows. This prevents the coverage check from running on PRs that only touch non-code files. Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Original prompt from michael.d.starch
|
🤖 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:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Coverage report — base
|
| Module | Line | Function | Branch |
|---|---|---|---|
CFDP/Checksum |
71.15 | 57.14 | 44.44 |
Drv/AsyncByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/ByteStreamBufferAdapter |
100.00 | 100.00 | 80.00 |
Drv/Ip |
44.32 | 57.38 | 21.25 |
Drv/TcpClient |
75.00 | 100.00 | 31.03 |
Drv/TcpServer |
84.72 | 100.00 | 44.26 |
Drv/Udp |
68.09 | 90.91 | 28.12 |
Fw/Buffer |
81.25 | 89.47 | 43.75 |
Fw/DataStructures |
98.48 | 97.14 | 57.37 |
Fw/Dp |
94.83 | 96.67 | 47.92 |
Fw/FilePacket |
75.24 | 89.06 | 41.43 |
Fw/Log |
71.43 | 72.41 | 34.48 |
Fw/Logger |
100.00 | 100.00 | 62.50 |
Fw/SerializableFile |
90.00 | 100.00 | 44.44 |
Fw/Time |
88.62 | 85.48 | 50.00 |
Fw/Tlm |
53.76 | 60.00 | 24.29 |
Fw/Types |
54.63 | 57.49 | 29.48 |
Os |
18.82 | 20.59 | 12.70 |
Os/Generic |
89.10 | 96.30 | 48.02 |
Os/Generic/Types |
92.45 | 91.67 | 60.94 |
Os/Posix |
61.04 | 83.72 | 40.18 |
Svc/ActiveRateGroup |
100.00 | 100.00 | 60.87 |
Svc/ActiveTextLogger |
79.05 | 90.00 | 53.85 |
Svc/AssertFatalAdapter |
94.55 | 100.00 | 59.09 |
Svc/BufferAccumulator |
88.00 | 94.12 | 54.79 |
Svc/BufferLogger |
92.62 | 86.96 | 57.46 |
Svc/BufferManager |
99.05 | 100.00 | 55.93 |
Svc/BufferRepeater |
91.67 | 100.00 | 55.56 |
Svc/ChronoTime |
100.00 | 100.00 | 50.00 |
Svc/CmdDispatcher |
96.97 | 91.67 | 52.60 |
Svc/CmdSequencer |
93.89 | 97.12 | 56.64 |
Svc/CmdSplitter |
100.00 | 100.00 | 54.55 |
Svc/ComLogger |
97.37 | 91.67 | 59.15 |
Svc/ComSplitter |
100.00 | 100.00 | 66.67 |
Svc/ComStub |
98.51 | 100.00 | 59.76 |
Svc/DpCatalog |
78.00 | 100.00 | 42.92 |
Svc/DpManager |
97.44 | 100.00 | 52.27 |
Svc/DpWriter |
97.58 | 90.00 | 62.07 |
Svc/FileDownlink |
84.15 | 90.91 | 45.29 |
Svc/FileManager |
88.41 | 93.33 | 50.45 |
Svc/FileUplink |
92.78 | 96.55 | 54.78 |
Svc/FileWorker |
92.33 | 100.00 | 45.11 |
Svc/FprimeDeframer |
100.00 | 100.00 | 58.04 |
Svc/FprimeFramer |
100.00 | 100.00 | 42.19 |
Svc/FprimeRouter |
88.89 | 100.00 | 48.65 |
Svc/FpySequencer |
86.41 | 99.02 | 51.67 |
Svc/GenericHub |
100.00 | 100.00 | 50.29 |
Svc/Health |
100.00 | 100.00 | 60.00 |
Svc/LinuxTimer |
97.06 | 100.00 | 47.06 |
Svc/OsTime |
70.00 | 83.33 | 37.80 |
Svc/PassiveRateGroup |
100.00 | 100.00 | 52.50 |
Svc/PolyDb |
100.00 | 100.00 | 42.50 |
Svc/PosixTime |
100.00 | 100.00 | 50.00 |
Svc/PrmDb |
92.20 | 90.00 | 51.29 |
Svc/RateGroupDriver |
100.00 | 100.00 | 62.50 |
Svc/SeqDispatcher |
73.08 | 80.00 | 49.18 |
Svc/StaticMemory |
100.00 | 100.00 | 55.00 |
Svc/SystemResources |
98.63 | 100.00 | 55.00 |
Svc/TlmChan |
82.40 | 85.71 | 48.57 |
Svc/TlmPacketizer |
92.54 | 100.00 | 54.30 |
Svc/Version |
96.10 | 100.00 | 54.91 |
Utils |
19.93 | 26.44 | 14.53 |
Utils/Types |
92.11 | 95.83 | 58.45 |
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/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, Utils/Hash, 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
* Drv: rename hostname to ipv4Address * Fix clang-format violations * Fix clang-format violations * formatting --------- Co-authored-by: not4right <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> Co-authored-by: Not4right <wordsbyeyes@gmail> Co-authored-by: M Starch <LeStarch@googlemail.com>
m_path stored a raw const CHAR* to the last path passed to open(), but no production code ever read it — there is no public accessor, and the member was only inspected by the unit test harness via friend access. The raw pointer also introduced a latent dangling-pointer hazard: the caller's string buffer could go out of scope while the file remained open, leaving m_path pointing at freed memory. The test code had an explicit workaround for this (resetting m_path after ConstStringBase opens). Changes: - Remove m_path from File class (declaration, open/close/copy/assign) - Update FileRules.cpp assertions to use tester's own m_current_path shadow state instead of reading m_path through the friend - Remove the dangling-pointer workaround in OpenFileCreateString Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
16fc313 to
79a53e9
Compare
2 participants
Change Description
Remove the private
const CHAR* m_pathmember fromOs::File. It was set onopen()and cleared onclose(), but never read by any production code — no public accessor exists.- const CHAR* m_path = nullptr; //!< Path last openedRemoved from: declaration,
open(),close(), copy ctor,operator=.Updated
FileRules.cpptest assertions to use the tester's ownm_current_pathshadow state instead of readingm_paththrough thefrienddeclaration. Removed the dangling-pointer workaround inOpenFileCreateString::action.Rationale
m_pathstored a rawconst CHAR*(not a copy), creating a latent dangling-pointer hazard if the caller's buffer went out of scope while the file remained open. The test code had an explicit workaround for this.friendaccess.Testing/Review Recommendations
m_pathhave been replaced with equivalent checks against the tester's own shadow statem_current_path/m_mode).SyntheticFileData::m_path(a separatestd::stringin the test harness) is intentionally left intact — it's the shadow filesystem's own bookkeeping.Future Work
None.
AI Usage (see policy)
AI was used to audit all usages of
m_pathacross the codebase and generate the removal patch.IAMAI
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/6ac403f622394d32913a28b4992c6716