-
Notifications
You must be signed in to change notification settings - Fork 202
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
Fix #778, add cfe assert and example lib #779
Conversation
7c34582
to
343f60d
Compare
d1f3cde
to
f061d5d
Compare
Rebased to main -- ready for review |
CCB 2020-07-29, APPROVED |
Integration Candidate 2020-07-29
@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. |
f061d5d
to
8ddd1e5
Compare
@skliper the 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.
8ddd1e5
to
f18a935
Compare
Correction - previous commit only got cfe_assert, commit f18a935 gets both. This should be good! |
One more update -- commit 4016605 cleans up the whitespace and reflects running the code through |
@astrogeco priority to get this merged. Walt is working of Joe's branch right now which isn't ideal. |
Merged into IC |
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) |
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. |
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. |
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.