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

Iox #454 create test file for iceoryx roudi and roudi app #587

Conversation

chiranjeevimaddi
Copy link
Contributor

@chiranjeevimaddi chiranjeevimaddi commented Mar 1, 2021

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. Branch follows the naming format (iox-#123-this-is-a-branch)
  4. 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)
  5. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  6. Relevant issues are linked
  7. Add sensible notes for the reviewer
  8. All checks have passed (except task-list-completed)
  9. Assign PR to reviewer

Notes for Reviewer

Test for a 2nd run will be done in #621.

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

JimmyBch and others added 13 commits March 1, 2021 20:47
@elfenpiff elfenpiff requested a review from marthtz March 2, 2021 16:14

auto cmdLineArgs = cmdLineParser.parse(numberOfArgs, args);

IceoryxRoudiApp_Child roudi(cmdLineArgs.value(), iox::RouDiConfig_t().setDefaults());
Copy link
Contributor

@elfenpiff elfenpiff Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do not want this kind of behavior or is there a use case that the RoudiApp should be instantiated twice in the same process?! @budrus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this come from the current implementation. The RouDiLock is located in IceOryxRouDiComponents.
The Components are only instantiated when you call IceOryxRouDiApp::run(). Here in this case only the class is instantiated... Maybe the lock should be relocated to fire earlier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, nothing bad happens until run is called, so this is no problem. What's instantiated is just a small shell around some configuration data, the actual roudi is created in run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the lock's description is Locks the socket for preventing multiple start of RouDi, so, is the intention just to prevent run or to instantiate it? In case of run-only another test for that is missing?!

Copy link
Member

@elBoberido elBoberido Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my point of view it should only be run, else you cannot execute ./iox-roudi -v anymore when RouDi is running. All the stuff that can break if two instances are running happens in the run method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense and I'm fine with that. So, is the PR complete now or should a 2nd run be tested?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the dev meeting, a test for a 2nd run will be done in #621.

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the RoudiApp be instantiatable twice in the same process?

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #587 (c71382c) into master (6c79f0c) will increase coverage by 1.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
+ Coverage   71.80%   73.16%   +1.35%     
==========================================
  Files         306      309       +3     
  Lines       10453    10654     +201     
  Branches     1959     1945      -14     
==========================================
+ Hits         7506     7795     +289     
+ Misses       2157     2097      -60     
+ Partials      790      762      -28     
Flag Coverage Δ
unittests 73.16% <ø> (+1.35%) ⬆️

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

Impacted Files Coverage Δ
...posh/include/iceoryx_posh/internal/popo/sample.inl 78.78% <0.00%> (-5.09%) ⬇️
.../include/iceoryx_utils/internal/units/duration.inl 94.70% <0.00%> (-3.95%) ⬇️
...ude/iceoryx_posh/internal/popo/base_subscriber.inl 86.00% <0.00%> (-1.50%) ⬇️
...nclude/iceoryx_dds/internal/gateway/dds_to_iox.inl 51.35% <0.00%> (-1.43%) ⬇️
iceoryx_posh/source/roudi/roudi.cpp 59.58% <0.00%> (-1.26%) ⬇️
iceoryx_posh/source/popo/trigger.cpp 91.66% <0.00%> (ø)
iceoryx_posh/source/roudi/port_pool.cpp 100.00% <0.00%> (ø)
iceoryx_utils/source/cxx/deadline_timer.cpp 100.00% <0.00%> (ø)
tools/introspection/source/introspection_app.cpp 0.00% <0.00%> (ø)
iceoryx_utils/include/iceoryx_utils/cxx/vector.hpp 100.00% <0.00%> (ø)
... and 72 more

@mossmaurice mossmaurice added the test A module/integration/stress/etc test for a component label Mar 18, 2021
@marthtz marthtz merged commit 16691b4 into eclipse-iceoryx:master Mar 19, 2021
marthtz added a commit to boschglobal/iceoryx that referenced this pull request May 12, 2021
…-create-test-file-for-iceoryx-roudi-and-roudi-app

Iox eclipse-iceoryx#454 create test file for iceoryx roudi and roudi app
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test A module/integration/stress/etc test for a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancing Tests in Roudi
7 participants