Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #778, add cfe assert and example lib #779

Merged
merged 3 commits into from
Aug 13, 2020

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Add a module for functional testing called "cfe_assert".
This is using the UT assert object code and linking it with some CFE glue so it is loadable as a CFE library.
Also included is the start of an example for CFE testing, which just calls some basic ES appid functions.

Fixes #778

Testing performed
Build and sanity check CFE.
Load these new test apps/libs and confirm all tests pass.

Expected behavior changes
This is all NEW test code which is compiled as separate modules and only loaded on demand. It is independent of existing FSW or other software modules.

System(s) tested on
Ubuntu 20.04

Additional context
A similar example for testing PSP will be included in PSP repo, and apps/libs can provide the same with the app/lib.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey linked an issue Jul 16, 2020 that may be closed by this pull request
@astrogeco astrogeco force-pushed the integration-candidate branch 3 times, most recently from 7c34582 to 343f60d Compare July 26, 2020 04:21
@jphickey
Copy link
Contributor Author

Rebased to main -- ready for review

@jphickey jphickey marked this pull request as ready for review July 29, 2020 14:40
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jul 29, 2020
@jphickey jphickey changed the base branch from integration-candidate to main July 29, 2020 14:42
@astrogeco astrogeco added this to the 7.0.0 milestone Jul 29, 2020
@astrogeco
Copy link
Contributor

CCB 2020-07-29, APPROVED

@astrogeco astrogeco added cFS-Caelum and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 5, 2020
@skliper
Copy link
Contributor

skliper commented Aug 11, 2020

@jphickey what was the directory structure you had in mind here? I don't quite follow the reasoning behind "fsw" (means flight software to me, which I wouldn't consider applicable marking for test code)... vs just cfe_test/src.

@jphickey
Copy link
Contributor Author

@skliper the fsw was just a remnant from cloning the directory structure. Note that one could load this into their flight system - it is just a regular CFE app at the end of the day. I agree no real reason to do so though, and I'm fine with dropping the fsw subdir.

Commit 8ddd1e5 rebases to the current "main" branch, squashes, and gets rid of the unnecessary dir layer.

Provides ability to load UT assert as a CFE app, and an example
of using this to test some basic CFE ES functions.
@jphickey
Copy link
Contributor Author

Correction - previous commit only got cfe_assert, commit f18a935 gets both. This should be good!

@jphickey
Copy link
Contributor Author

One more update -- commit 4016605 cleans up the whitespace and reflects running the code through clang-format.

@skliper skliper added the Priority: Mission Feature or bug related to stakeholder needs label Aug 13, 2020
@skliper
Copy link
Contributor

skliper commented Aug 13, 2020

@astrogeco priority to get this merged. Walt is working of Joe's branch right now which isn't ideal.

@astrogeco astrogeco changed the base branch from main to integration-candidate August 13, 2020 13:50
@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Aug 13, 2020
@astrogeco astrogeco merged commit 10ccb16 into nasa:integration-candidate Aug 13, 2020
@astrogeco
Copy link
Contributor

@astrogeco priority to get this merged. Walt is working of Joe's branch right now which isn't ideal.

Merged into IC

@jphickey
Copy link
Contributor Author

Unfortunately -- some additional review has noted a race condition in the current code. Can we revert this from the IC? As this PR got closed I will submit a new one.

(FWIW - I still say we shouldn't be closing PRs until they reach "main" because the IMO whole point of "integration-candidate" is to pre-test PRs BEFORE claiming we've "merged" them -- but that's a whole other topic)

@skliper
Copy link
Contributor

skliper commented Aug 14, 2020

FWIW - I still disagree on the approach of leaving PRs open until merged to main due to the confusion it creates based on introducing 2 places the code could change, the PR or the IC branch. One main flow keeps "ownership" clear. It was merged to IC, fix it in IC (or add a new PR). IMO any changes after a PR has been merged to IC belong in IC (hotfixes), and/or new PRs.

@jphickey
Copy link
Contributor Author

My major gripe is that a PR being evaluated/tested and is determined to be broken should be rejected and marked as such (closed, not merged) so its obvious in the future that this it wasn't the accepted resolution to the issue. It shouldn't require digging through comments to figure that out. The misleading final status on PR is very confusing to me. No desire to argue about it again though.

Replacement PR is submitted in #804. I recommend reverting this one and making an IC with that one instead.

@skliper skliper removed this from the 7.0.0 milestone Aug 21, 2020
@jphickey jphickey deleted the fix-778-cfe-assert branch September 29, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB cFS-Caelum invalid Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UT Assert library for CFE functional tests
3 participants