-
Notifications
You must be signed in to change notification settings - Fork 217
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 #390, #392, osal select API unit tests and fixes #409
Fix #390, #392, osal select API unit tests and fixes #409
Conversation
Note this doesn't actually address #377, I probably didn't do a good job describing the issue. The "functional tests" that are missing are the ones similar to those in src/tests which don't use mocks/stubs. The functional tests needed for #377 are against the API, and should demonstrate the functions work as intended. The tests here are the "coverage test" part of unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here are good, but shouldn't mark #377 as fixed (see my other comment).
Nitpick - preference would be to separate flight code changes and unit test changes into separate commits
No these do not use stubs, they do confirm the functional behavior of the Select API. |
Wait, I'm sure I've been told to commit tests when I commit flight code... :D I can certainly re-split-out the null pointer check and enum move if the CCB would prefer as a separate pull. I did give a heads-up at last CCB that I would do a pull request incorporating all changes together, and heard no objections. |
Not a separate pull request, just separate commits. I have to work on explaining what's in my head. Another concept I describe poorly is "one topic per commit"... and grouping commits in a pull request. |
Really just a nit in this case since the changes are very easy to review, straight forward, etc. More complex changes where there's documentation, unit tests, flight code, etc my preference is smaller commits group into a single pull request. Probably useful information for a contributor's guide... |
Worth a discussion at the CCB... I didn't immediately see where it shows that the select blocks as it's supposed to until a file becomes readable or writable, or a demonstration of the timeout and verification of reasonable timing. |
Should note that RTEMS doesn't really support |
CCB 20200415 - APPROVED |
Integration Candidate: 2020-04-15
Describe the contribution
fixes #390, fixes #392 -- adds unit tests for OSAL select API
Partially addresses #377, but full solution is a functional test that includes an operational scenario (actually select on a resource, confirm it pends until that resource is available).. bonus points if it illustrates the differences between OS's so it's clear from a user perspective what will work and what wont.
Testing performed
Built unit tests, tests passed.
Expected behavior changes
Externalizes enum for SelectSingle, ensures that pointers passed to SelectFd...() APIs are not null, ensures that pointer to SelectSingle is not null.
System(s) tested on
Debian 9
Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov