Iox #27 client port implementation and test part 2

[elBoberido]

Pre-Review Checklist for the PR Author

1. Code follows the coding style of CONTRIBUTING.md
2. Tests follow the best practice for testing
3. Changelog updated in the unreleased section including API breaking changes
4. Branch follows the naming format (iox-#123-this-is-a-branch)
5. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit messages are signed (git commit -s)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
6. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
7. Relevant issues are linked
8. Add sensible notes for the reviewer
9. All checks have passed (except task-list-completed)
10. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Relates to Request/Response communication with iceoryx #27

[codecov]

Codecov Report

Merging #863 (41885ec) into master (3550c66) will increase coverage by 0.81%.
The diff coverage is 93.01%.

@@            Coverage Diff             @@
##           master     #863      +/-   ##
==========================================
+ Coverage   74.79%   75.60%   +0.81%     
==========================================
  Files         334      335       +1     
  Lines       12020    12215     +195     
  Branches     2017     2037      +20     
==========================================
+ Hits         8990     9235     +245     
+ Misses       2255     2195      -60     
- Partials      775      785      +10     

Flag	Coverage Δ
unittests	74.50% <93.01%> (+0.82%)	⬆️
unittests_timing	29.61% <0.00%> (-0.46%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lude/iceoryx_posh/internal/capro/capro_message.hpp	100.00% <ø> (ø)
...ryx_posh/internal/popo/ports/client_port_roudi.hpp	100.00% <ø> (+100.00%)	⬆️
...oryx_posh/internal/popo/ports/client_port_user.hpp	100.00% <ø> (+100.00%)	⬆️
...ryx_hoofs/source/error_handling/error_handling.cpp	50.00% <75.00%> (+4.83%)	⬆️
...x_posh/include/iceoryx_posh/iceoryx_posh_types.inl	80.64% <89.47%> (+13.97%)	⬆️
...lude/iceoryx_posh/internal/capro/capro_message.inl	90.24% <90.24%> (ø)
...eoryx_posh/source/popo/ports/client_port_roudi.cpp	94.82% <94.28%> (+94.82%)	⬆️
...nclude/iceoryx_dds/internal/gateway/iox_to_dds.inl	55.00% <100.00%> (-0.74%)	⬇️
iceoryx_hoofs/source/log/logger.cpp	64.70% <100.00%> (+4.70%)	⬆️
...ceoryx_posh/source/popo/ports/client_port_data.cpp	100.00% <100.00%> (+100.00%)	⬆️
... and 18 more

[elBoberido]

@budrus @elfenpiff @MatthiasKillat what do you think about this ideas?

@Indra5196 please also have a look at the additions to the design document

[elBoberido]

To view this with rendered diagrams use the three dots on the right and select View file

[mossmaurice]

Please move this into the switch case as default and you will earn a MISRA badge ;)

[elBoberido]

Moving this into default would mean we might miss a new message added to the enum. Without a default case, the compiler will complain. There is still the return after the switch, in order to catch errors when a dummy enum value was provided. In this case, it's clear that MISRA is for C type enums and not enum class from C++

[mossmaurice]

Ok, can you please double check the Axivion scan with our custom AUTOSAR ruleset?

[elBoberido]

ahem, let's assume I'm a new contributor ... how would I do that?

[mossmaurice]

This is describe in the CONTRIBUTING.md: Friendly ping one of the maintainers to get access to the dashboard ;)

@dkroenke will probably give everyone some heads-up before this goes live with every PR in a couple of weeks. I'd appreciate if you give it a try for this PR :)

[mossmaurice]

You can find the config files with all the rules in here

[elBoberido]

@mossmaurice according to dkroenke the dashboard currently only works with the master branch. We currently use this pattern (no default but return statement) in several places already, e.g. https://github.com/eclipse-iceoryx/iceoryx/blob/master/iceoryx_posh/source/popo/trigger.cpp#L32-L41.
Furthermore it is semantically the same as with the default case when all enum tags are handled and compiles down to the exact same assembly with the benefit of a compile time error instead of a silent fallback to default when the enum is extended.

Assuming our current rule set doesn't allow this, we need to change it anyway since it doesn't make sense and results in bad code where errors need to be catched by tests instead of the compiler.

[mossmaurice]

ASSERT_NE(requestHeader, nullptr)?

[elBoberido]

Like before, ASSERT_NE is not possible in this context. I would need to rewrite this completely if necessary

[mossmaurice]

Besides small nitpicks, LGTM