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 #413, Add coverage tests for pc-rtems #414

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Oct 16, 2023

Checklist

Describe the contribution

  • Fixes Coverage tests missing for pc-rtems #413
    • Adds coverage tests for pc-rtems implementation (some function are just placeholders anyway, but the coverage tests are there for users to amend for their implementation).
    • Minor clean-up of CMakeLists.txt files etc.

Testing performed
GitHub CI actions all passing successfully and local testing results in the following added coverage for pc-rtems:
Screenshot 2023-10-16 15 55 22

Expected behavior changes
No code changes - only adding new tests.

System(s) tested on
Debian GNU/Linux 11 (bullseye)
Current main branch of cFS bundle.

Contributor Info
Avi Weiss   @thnkslprpt

@thnkslprpt
Copy link
Contributor Author

thnkslprpt commented Oct 16, 2023

I wasn't able to easily set up the context enough to test CFE_PSP_WriteToCDS and CFE_PSP_ReadFromCDS fully - if someone can get it working I'll add it in.

@thnkslprpt thnkslprpt force-pushed the fix-413-add-pc-rtems-coverage-tests branch from 3900bc3 to 28909c9 Compare October 19, 2023 13:49
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Great work! Apologies it took so long to get to a review. A few minor nits below and recommend a rebase on main and should be good to go!

Eventually it'd be nice to restructure slightly to split into separate executables like apps do it (same change for the VxWorks ut's), and separate the shared test from the VxWorks one but I recommend getting this PR in before tackling those restructuring changes. I can also help w/ the final few uncovered lines (although lets get this in first).

@thnkslprpt thnkslprpt force-pushed the fix-413-add-pc-rtems-coverage-tests branch from 28909c9 to 5f24032 Compare December 9, 2023 02:17
@thnkslprpt thnkslprpt force-pushed the fix-413-add-pc-rtems-coverage-tests branch from 5f24032 to 3c11f3f Compare December 9, 2023 04:47
@skliper skliper added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 11, 2023
@skliper
Copy link
Contributor

skliper commented Dec 11, 2023

Recommending CCB review (and hopefully merge) so we can continue moving forward.

Future work: Minor coverage improvements, add to CI and enforce (already #18), stub out shared APIs and cover those in their own test cases, individual exe per file.

@dmknutsen dmknutsen added CCB:Approved Indicates Approval by CCB and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 14, 2023
@dmknutsen dmknutsen merged commit 4ca6c91 into nasa:main Dec 15, 2023
11 checks passed
@thnkslprpt thnkslprpt deleted the fix-413-add-pc-rtems-coverage-tests branch December 15, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates Approval by CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverage tests missing for pc-rtems
3 participants