Skip to content

Conversation

@m-aleem
Copy link
Contributor

@m-aleem m-aleem commented Jun 26, 2025

Related Issue(s) #3446
Has Unit Tests (y/n) Yes - Existing
Documentation Included (y/n) N/A

Change Description

I locally updated fprime/cmake/settings.cmake to change -DPROTECTED=protected -DPRIVATE=private. This causes additional build errors beyond all our updates in manual code for PRIVATE->private and PROTECTED->protected due to auto-coded files which use PRIVATE, PROTECTED. This set of changes is associated with fixing Fpp so that the UTs in this directory can build and pass.

Approach/changes in this case was:

  1. In components, re-name existing Tester.hpp/cpp to follow friendship name pattern and allow private/protected access (example: ActiveTestTester.hpp/cpp) then update the existing Tester.hpp to point to the new file in order to keep the test structure
  2. Re-name Fpp/dp tests to DpTestTester to follow friendship name pattern and allow private/protected access
  3. Refactor all FppTest/state_machine/internal_instance/state files to move tests into their own <x>Tester.hpp/cpp classes, again to follow friendship name pattern and allow private/protected access

Rationale

#3446

Future Work

  1. Note that this PR does not include the update to fprime/cmake/settings.cmake itself

@m-aleem m-aleem changed the title Updates for FPP corresponding to UT cmake udpate for protected/private Updates for FPP corresponding to UT cmake update for protected/private Jun 26, 2025
@m-aleem m-aleem closed this Jun 26, 2025
@m-aleem m-aleem reopened this Jun 26, 2025
@m-aleem m-aleem changed the title Updates for FPP corresponding to UT cmake update for protected/private Updates for FPPTest corresponding to UT cmake update for protected/private Jun 26, 2025
@m-aleem m-aleem marked this pull request as ready for review June 26, 2025 19:09
@LeStarch LeStarch requested review from Kronos3, bocchino and Copilot and removed request for Copilot June 26, 2025 22:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the FPPTest unit tests to support the recent cmake update for protected/private by moving inline test functions out of production classes and into dedicated Tester classes. Key changes include:

  • Renaming and relocating test functions (e.g., in StateToState, BasicGuard, BasicInternal, etc.) into corresponding Tester classes that are declared as friends to access private/protected members.
  • Updating include directives and CMakeLists.txt files to incorporate the new tester files.
  • Removing redundant inline test methods from production source to improve maintainability.

Reviewed Changes

Copilot reviewed 110 out of 110 changed files in this pull request and generated no comments.

File Description
Multiple source files under FppTest/state_machine/internal_instance/state/ Moved inline test methods from production classes into separate Tester classes and updated friend declarations accordingly.
CMakeLists.txt Updated to include new tester source files ensuring all new test classes are integrated into the build.
Comments suppressed due to low confidence (2)

FppTest/state_machine/internal_instance/state/StateToState.cpp:70

  • The removal of inline test methods from the production StateToState class is a good step for separation of concerns; please ensure that the corresponding StateToStateTester class provides equal or greater test coverage to maintain robustness.
    this->m_smStateStateToState_actionHistory.push(signal, ActionId::ENTER_S5);

@LeStarch
Copy link
Collaborator

@bocchino and @Kronos3 would one of you review

@m-aleem m-aleem marked this pull request as draft July 9, 2025 22:28
@m-aleem m-aleem marked this pull request as ready for review July 10, 2025 05:21
@m-aleem
Copy link
Contributor Author

m-aleem commented Jul 10, 2025

@bocchino Incorporated the changes we discussed at todays tag up, this should be ready for-review/merge (and, once merged, the additional steps to coordinate F-Prime/FPP for this task are in a comment in nasa/fpp#754)

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I noticed that some of the changes required reformatting, so I reformatted all the code. That caused major changes in some files that had not previously been formatted in the standard way.

@LeStarch @Kronos3 if you want to review this further, I can make a branch of nasa/devel with just the reformatting, for comparison. Let me know.

@LeStarch LeStarch merged commit 9211a04 into nasa:devel Jul 18, 2025
51 checks passed
@m-aleem m-aleem deleted the devel-3446-am branch July 22, 2025 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants