iox-#415 Rename variable and test case name in ServiceRegistry_test and fix bug in ServiceDescription c'tor

[mossmaurice]

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

• Fixes left-over findings from Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860
  • Rename sut in ServiceRegistry_test
  • Rename one test case in ServiceRegistry_test
  • Fix bug in ServiceDescription::ServiceDescription(const cxx::Serialization& serial)
  • Replace bool with void as return type of ServiceRegistry::remove

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

• Closes partly service registry as a built-in topic #415

[codecov]

Codecov Report

Merging #917 (2815364) into master (d193464) will increase coverage by 1.52%.
The diff coverage is 68.57%.

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
+ Coverage   76.02%   77.55%   +1.52%     
==========================================
  Files         334      334              
  Lines       12258    12370     +112     
  Branches     2052     1840     -212     
==========================================
+ Hits         9319     9593     +274     
+ Misses       2147     2144       -3     
+ Partials      792      633     -159     

Flag	Coverage Δ
unittests	76.45% <68.57%> (+1.49%)	⬆️
unittests_timing	29.95% <0.00%> (+0.23%)	⬆️

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

Impacted Files	Coverage Δ
...include/iceoryx_posh/capro/service_description.hpp	100.00% <ø> (ø)
...posh/include/iceoryx_posh/runtime/posh_runtime.hpp	100.00% <ø> (ø)
iceoryx_posh/source/roudi/service_registry.cpp	96.87% <ø> (-0.05%)	⬇️
iceoryx_posh/source/roudi/roudi.cpp	57.07% <20.00%> (+7.31%)	⬆️
iceoryx_posh/source/runtime/posh_runtime_impl.cpp	65.72% <50.00%> (-0.42%)	⬇️
iceoryx_posh/source/capro/service_description.cpp	97.84% <93.75%> (+3.16%)	⬆️
iceoryx_posh/source/popo/client_options.cpp	100.00% <100.00%> (ø)
...e/relocatable_pointer/base_relocatable_pointer.cpp	0.00% <0.00%> (-15.16%)	⬇️
iceoryx_hoofs/source/cxx/helplets.cpp	63.63% <0.00%> (-6.37%)	⬇️
iceoryx_hoofs/source/log/logging.cpp	100.00% <0.00%> (ø)
... and 69 more

[elfenpiff]

I reviewed it and to address review findings in a follow up pull request is a horrible idea since now a reviewer has to click through all the review findings to make sure they were addressed. And as it turns out not all of them were addressed and even not tracked within the issue.

Here I list them, please fix every issue and open point from the former PR otherwise we create another PR to address again review findings from a former PR and it is getting harder and harder to see which issue was handled and which not:

• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)
• Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)

Either address the issue in this PR or add them as todo in the issue with a link to the discussion.
Furthermore @elBoberido and @MatthiasKillat should be the ones who approve this PR since they where the initial approvers

[elfenpiff]

@mossmaurice Shouldn't the PR title be something like iox-#123 Address... since you checked Update the PR title Follow the same conventions as for commit messages ?

[mossmaurice]

I reviewed it and to address review findings in a follow up pull request is a horrible idea since now a reviewer has to click through all the review findings to make sure they were addressed.

Thanks for your feedback! I added the three findings that were fixed in this PR under "Notes for Reviewer".
TBH I think the iterative approach used here is not horrible, but exactly what we need in order to keep the PRs as small as possible. This is what we've done in the past and IMHO we should stick with it for future development.

And as it turns out not all of them were addressed and even not tracked within the issue.

I have the feeling that we have a different understanding on what is "an important finding" vs. what is "just nice to have"-personal-preference. I addressed all the findings in #860 and resolved them if there was no more feedback/answer from the original reviewer. All the important findings e.g. small bugs are tracked in #415 in a todo list. Usually, I tend to track smaller things or "nice-to-have" stuff with an inline comment /// @todo #123. Especially for pull requests that are open for several months, I would kindly ask you to do a re-review after ~1 months and take part in the discussion about a certain issue if you don't agree.

Here I list them, please fix every issue and open point from the former PR otherwise we create another PR to address again review findings from a former PR and it is getting harder and harder to see which issue was handled and which not:

For easier reference let me replace the links with some short description:

1. Connect MAX_NUMBER_OF_SERVICES with MAX_SERVICE_DESCRIPTIONS

Tracked via an inline comment.

2. bool remove(ServiceDescription&) or void remove(ServiceDescription&)

@MatthiasKillat and myself were both ok with the bool. However, I see your point, will adapt it.

3. registry.find("bla", "blubb", "fuu") vs. registry.find(result, "bla", "blubb", "fuu")

As written in the thread: @MatthiasKillat and myself concluded something like this is always a trade-off. As this is an internal API that will never be used by the user directly it's not worth the hassle IMHO and can also be easily adapted if we experience it to be error-prone.

4. Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860 (comment)

Resolved, comment from when the predecessor PR was not merged to master yet.

5. iterator not part of for loop head

Done on purpose to show it goes hand in hand with the index. This will be refactored once we integrate #859

6. Counted two occurrences of m_serviceMap.equal_range

@MatthiasKillat told me he won't support equal_range with #859 Hence, this will be refactored soonish.

Furthermore @elBoberido and @MatthiasKillat should be the ones who approve this PR since they where the initial approvers

I have the feeling that we need a vote for a BDFL? 😏 Kidding aside, especially during vacation time we can't always wait for everyone to return. In general, I try to address & merge my open pull requests ASAP but sometimes the workload does not allow this. Let's improve our review speed and finding-fix-time with the next PRs 😊 For the record #860 was open 72 days.

[elBoberido]

Just some minor things

[dkroenke]

Since there is ServiceDescription::deserialize introduced and the serialization constructor removed, it would be good to have an entry in the Changelog with a description of the changes in the public API.