Skip to content

Conversation

@m-aleem
Copy link
Contributor

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

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

Change Description

Updates to fprime/Svc/FpySequencer* to change:

  • PRIVATE -> private
  • PROTECTED -> protected
  • Add friend tester helpers, as needed

Rationale

#3446

Testing/Review Recommendations

See questions about Spell Checking CI failure in comment below

Future Work

Additional PRs to be opened for further updates corresponding to #3446

@m-aleem m-aleem requested a review from LeStarch June 16, 2025 19:33
@m-aleem
Copy link
Contributor Author

m-aleem commented Jun 16, 2025

CI Spell check is unhappy with the following:

However, these are calls to existing getstatements and getfooter methods (of Fpy::Sequence)

Is there a recommended way to handle this? (I.e. (1) is this OK or (2) should I make a new ticket for this against Fpy::Sequence or (3) incorporate update to the method re-naming as part of this work or (4) something else?)

@m-aleem m-aleem marked this pull request as ready for review June 16, 2025 20:05
@LeStarch LeStarch requested a review from zimri-leisher June 16, 2025 21:05
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I asked @zimri-leisher to look at this as the original author. For now, this is a reasonable solution and deemed less "ugly" then a friendship statement for each test subclass created by TEST_F.

In the future, all black box testing can use TEST_F but white box testing must add methods to the test for white box access.

@zimri-leisher
Copy link
Collaborator

This is an unfortunately large amount of boilerplate... there would be less if we used the FRIEND_TEST macro in gtest. If you guys have deemed that more ugly than this solution, okay fine, but it would require fewer lines of code changed. It would probably be what I would have chosen, but if you've already gone and done this it's fine. I appreciate the large amount of work you've done here, @m-aleem

I wish we weren't being put in this situation, but I suppose it is the right choice to switch to friend classes from private/public redefines.

Now I just feel bad for using TEST_F at all...

@LeStarch
Copy link
Collaborator

@zimri-leisher you tried something new, which has very real benefits. Do not feel bad. This is just the normal iteration and learning process.

For example, for purely black-box testing (ports/command and output ports, events, telemetry) TEST_F is an unambiguous win. It just has some minor drawbacks for white-box internal state detection. I personally am adopting TEST_F when my interface is clean enough that black-box testing is sufficient!

@LeStarch LeStarch merged commit 9c5c024 into nasa:devel Jun 17, 2025
47 checks passed
@m-aleem m-aleem deleted the devel-3446-f-v2 branch June 17, 2025 21:53
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