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

Impossible to test OS_CONSOLE_ASYNC false case #957

Closed
skliper opened this issue Apr 11, 2021 · 4 comments · Fixed by #959 or #975
Closed

Impossible to test OS_CONSOLE_ASYNC false case #957

skliper opened this issue Apr 11, 2021 · 4 comments · Fixed by #959 or #975
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Apr 11, 2021

Is your feature request related to a problem? Please describe.
Now that OS_CONSOLE_ASYNC is defined locally, it's impossible to functionally test false case or get full branch coverage on OS_ConsoleCreate_Impl without modifying the code under test:

/*
* By default the console output is always asynchronous
* (equivalent to "OS_UTILITY_TASK_ON" being set)
*
* This option was removed from osconfig.h and now is
* assumed to always be on.
*/
#define OS_CONSOLE_ASYNC true

local->is_async = OS_CONSOLE_ASYNC;
if (local->is_async)

Describe the solution you'd like
Either need to be able to exercise this option or remove it.

Describe alternatives you've considered
None

Additional context
Prevents full branch coverage

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 6.0.0 milestone Apr 11, 2021
@jphickey
Copy link
Contributor

I'm trying to recall why this was removed from osconfig.h ... It is still relevant/preferable to run in this mode for debugging situations. Many times OS_printf is used for debugging and if the actual output is decoupled, it can change the order with which things appear. It also saves the memory cost of another task+stack in constrained environments.

If I understand correctly the issue here is limited to the "create_impl" function only - since it can't easily be tested with the flag as false. Maybe a compromise solution would be to move the boolean from the impl layer to the shared layer, which would allow the impl coverage test to set it both ways at least.

@skliper
Copy link
Contributor Author

skliper commented Apr 12, 2021

I'd rather make it an option that can be tested functionally or remove it. Coverage testing with overrides doesn't prove all the logic works together.

Option - ifndef make it async=true.

@skliper
Copy link
Contributor Author

skliper commented Apr 12, 2021

Or relate it to OS_DEBUG or similar, and/or note the expected context/reasoning.

@jphickey
Copy link
Contributor

I consider it similar to the shell switch or something like that - for FSW the user chooses which way to configure it and then it gets compiled in that config, but for coverage test - due to the design and its "backdoor" methods of running things directly, it can still test it both ways, regardless of how its configured in FSW.

Right now even that's not possible because of the proximity to the definition and the runtime check. If they were in separate units, this would be easily doable.

Can also resurrect this as an osconfig option. As I said, I don't recall why it was removed, but might have been before osconfig.h was converted to osconfig.cmake. In the current design there shouldn't be a problem of having this option, and defaulting it to true.

I'd rather not tie it to OS_DEBUG - although that's one justification for changing it - simply saving the cost of an extra task+stack might be worthwhile to some users.

jphickey added a commit to jphickey/osal that referenced this issue Apr 12, 2021
Puts the "async" option into the shared layer instead of the impl layer.
This allows both options to be coverage tested and also allows a bit more
of the logic to be common instead of duplicated in the 3 implementations.

This also adds back an osconfig option to allow the user to elect this
mode at configuration time.
astrogeco added a commit that referenced this issue Apr 28, 2021
jphickey pushed a commit that referenced this issue Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants