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 #795, Add uncovered vxworks source files to coverage statistics #826

Merged

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Feb 19, 2021

Describe the contribution
Fix #795 - Adds all the currently possible source files for vxworks to coverage statistics and adds coverage tests

  • Renamed sockaddr* structures to deconflict from structure name in os-impl-bsd-sockets.c to make stubbing out the OS structures easier.
  • Compiling os-impl-bsd-sockets.c in coverage test with OS_NETWORK_SUPPORTS_IPV6 to cover that option also
  • Renamed bsd-select-stubs.c to sys-select-stubs.c since it's really stubbing sys/select.h not the bsd implementation of select

Testing performed
Build and execute unit tests, passed

Expected behavior changes
None, but coverage now includes all currently possible files in vxworks build

System(s) tested on

  • Hardware: cFS Dev Server
  • OS: Ubuntu 18.04
  • Versions: cFS Bundle main + this commit

Additional context
Not full coverage yet on os-impl-bsd-sockets.c (plan to keep improving prior to CCB)
Also will squash

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Feb 19, 2021
@skliper skliper added this to the 6.0.0 milestone Feb 19, 2021
@skliper
Copy link
Contributor Author

skliper commented Feb 19, 2021

@jphickey Is this along the lines of what you'd expect?

@jphickey
Copy link
Contributor

@jphickey Is this along the lines of what you'd expect?

Yes! This looks good!

The only comment is regarding the endian conversions - e.g. ntohl/s, etc - these are ones that I would consider falling back to "real" behavior - similar to memcpy/memset etc - if nothing is configured. Having it just return 0 by default might cause problems for some code. But in those cases I suppose someone can just be sure to set/register a return value in the test case.

@skliper
Copy link
Contributor Author

skliper commented Feb 22, 2021

...ntohl/s...

I think I left those out since host could be big or little endian, and preference might be a hook that emulates the real target? Just don't want testers to assume coverage testing w/ the test host endian is really appropriate in all cases.

@jphickey
Copy link
Contributor

I do agree that if there is any conditional which depends on output of an endian conversion, that the test case should explicitly set that output, rather than assuming the stub is going to work like the real one does. So it's probably fine the way it is.

@astrogeco astrogeco added CCB:2021-02-24 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Feb 24, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Feb 24, 2021

CCB:2021-02-24 APPROVED

  • Doesn't get to 100% coverage yet

@skliper
Copy link
Contributor Author

skliper commented Feb 25, 2021

See #831 for follow-on to finish adding coverage

@skliper skliper marked this pull request as ready for review February 25, 2021 13:17
@skliper
Copy link
Contributor Author

skliper commented Feb 25, 2021

@astrogeco squashed, ready for merge

@astrogeco astrogeco changed the base branch from main to integration-candidate March 1, 2021 22:04
@astrogeco astrogeco merged commit 8576598 into nasa:integration-candidate Mar 1, 2021
@skliper skliper deleted the fix795-config_coverage branch April 1, 2021 20:06
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
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.

Update coverage tests for VxWorks at minimum to include reporting of all code that could be included in build
3 participants