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 #17, update code coverage for fm_cmd_utils.c #33

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

havencarlson
Copy link
Contributor

@havencarlson havencarlson commented Jun 8, 2022

Checklist (Please check before submitting)

Describe the contribution

Testing performed
Steps taken to test the contribution:

  1. lcov --capture --rc lcov_branch_coverage=1 --directory build --output-file coverage_test.info
  2. lcov --rc lcov_branch_coverage=1 --add-tracefile coverage_base.info --add-tracefile coverage_test.info --output-file coverage_total.info
  3. genhtml coverage_total.info --branch-coverage --output-directory lcov

Expected behavior changes
no impact to behavior

System(s) tested on

  • OS: Ubuntu 18.04

Additional context
Add any other context about the contribution here.

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
Full name and company/organization/center of all contributors ("Personal" if individual work)

  • Note CLA's apply to software contributions.

@astrogeco
Copy link
Contributor

@havencarlson this needs a run through clang format to fix white space. You can also fix it manually by looking at the output of the format check workflow

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.

Comments apply to patterns throughout the change. Avoid UtAssert_True, UT_GetStubCount, etc in favor of the macros that standardize the report information. Ensure variables are initialized before use, etc.

unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Show resolved Hide resolved
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! Just a few minor updates recommended. Happy to chat if it helps.

unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
unit-test/fm_cmd_utils_tests.c Outdated Show resolved Hide resolved
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.

Perfect, great work!

@skliper
Copy link
Contributor

skliper commented Jun 17, 2022

@astrogeco - should be ready to merge... we'll need to update the workflow allowed missed branches/lines once these are all in.

@astrogeco
Copy link
Contributor

@chillfig Still need some format fixes

@astrogeco
Copy link
Contributor

astrogeco commented Jun 17, 2022

CCB:2022-06-15 APPROVED

@astrogeco astrogeco merged commit c651341 into nasa:main Jun 17, 2022
@skliper skliper added this to the Draco milestone Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants