Iox #454 enhancing coverage for class roudi and port manager in roudi folder #527

chiranjeevimaddi · 2021-01-29T06:37:45Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Code contributed by @JimmyBch @manel94

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

All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

All checkboxes in the PR checklist are checked or crossed out
Merge

References

Partly Closes Enhancing Tests in Roudi #454

Signed-off-by: Jimmy Belloche <[email protected]>

…roudi_portmanager Signed-off-by: Manel Melaouhi <[email protected]>

…d clean for class roudi_portmanager Signed-off-by: Manel Melaouhi <[email protected]>

…-roudi Signed-off-by: Jimmy Belloche <[email protected]>

Signed-off-by: Manel Melaouhi <[email protected]>

…T_THAT Signed-off-by: Manel Melaouhi <[email protected]>

Signed-off-by: Manel Melaouhi <[email protected]>

JimmyBch · 2021-01-29T08:10:12Z

iceoryx_posh/test/moduletests/test_roudi.cpp

@@ -0,0 +1,545 @@
+// Copyright (c) 2021 by Robert Bosch GmbH, Apex.AI Inc. All rights reserved.


Concerning this file, many EXPECT are missing. I am aware but I didn't have any idea how to really check the different tests. On my side, I think it is missing an errorHandler when number of elements is incorrect.

To which number of elements are you referring to?

Sorry, I was not so explicit. In the procesManager method of class Roudi (roudi.cpp), if the message does not contain the correct number of elements depending on the MqMessageType received, an error is set. In my opinion, errohandler could be used there.

ErrorHandler is not used right now. But, I guess it could be a potential improvement of the existing code, correct ?

I guess you refer to the method processMessage right?
Yes the errorhandler should be used there, but only with level Moderate because RouDi can proceed and the request from the runtime is not processed.
There is even in one case the errorhandler used:

iceoryx/iceoryx_posh/source/roudi/roudi.cpp

Line 230 in f7fee3c

Error::kPORT_MANAGER__INTROSPECTION_MEMORY_MANAGER_UNAVAILABLE, nullptr, iox::ErrorLevel::MODERATE);

@elBoberido does it make sense to have an error like Error::kROUDI__WRONG_NUMBER_PARAMETERS_MQMESSAGE? Or do you want to refactor that too?

I think that's not necessary on the roudi side. It just hast to be communicated back to the application since this is input from an application and therefore the error handler should also be called on the application side.

Does it mean the method process manager should return something ?

This is something that will heavily be refactored with #332 in the future to give the application more information what's going on. As a side effect, this makes testing also easier.
Currently there is no response to the application if the number of parameter don't match. With #332 that will change and an error response will be sent back.
What's even worse, calling the error handler with Error::kPORT_MANAGER__INTROSPECTION_MEMORY_MANAGER_UNAVAILABLE is definitely wrong and should be removed. The application just provided us with the wrong number of parameter.
Currently the RouDi class is not really good testable.

dkroenke

Just a few comments, detailed review will follow....

dkroenke · 2021-01-29T08:20:34Z

iceoryx_posh/test/moduletests/test_roudi.cpp

@@ -0,0 +1,545 @@
+// Copyright (c) 2021 by Robert Bosch GmbH, Apex.AI Inc. All rights reserved.


To which number of elements are you referring to?

dkroenke · 2021-01-29T08:21:11Z

iceoryx_posh/test/moduletests/test_roudi.cpp

+namespace test
+{
+/// @req IOX_SWRS_112, IOX_SWRS_200
+/// @brief Test goal: "This test suite verifies that the BaseClass function is verified"


Very good! Please fill out the template.

dkroenke · 2021-01-29T08:24:56Z

iceoryx_posh/test/moduletests/test_roudi.cpp

+    m_roudiApp->shutDownTest();
+}
+
+/// @note name of the Testcase shall describe the test case in detail to avoid additional comments


can be removed when the name is meaningful

elfenpiff

Most of the tests are missing the verification of the behavior. Please add it.

JimmyBch · 2021-01-29T09:03:38Z

Most of the tests are missing the verification of the behavior. Please add it.

Yes, I let a comment about it above :)

elBoberido

All the tests without an EXPECT give a false feeling of safety because the line coverage is high, but except not crashes, nothing is verified. The RouDi class is currently not easily testable and needs a refactoring, which will happen in the near future.

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

iceoryx_posh/test/moduletests/test_roudi.cpp

Signed-off-by: Jimmy Belloche <[email protected]>

…nager Signed-off-by: Manel Melaouhi <[email protected]>

…g to the container Signed-off-by: Manel Melaouhi <[email protected]>

…nts and replace EXPECT_* with ASSERT_* for two tests Signed-off-by: Manel Melaouhi <[email protected]>

…ing needed Signed-off-by: Jimmy Belloche <[email protected]>

chiranjeevimaddi · 2021-02-04T12:03:00Z

working on fixing the build

…yx#454-Enhancing-coverage-for-class-roudi-and-port-manager-in-roudi-folder Signed-off-by: Chiranjeevi Maddi (RBEI/EBB1) <[email protected]>

manel94 · 2021-02-04T14:17:16Z

@elBoberido i pushed some updates :)

elBoberido

Looking at the existing test in this file makes it a bad example on how to write tests. It seems this file can count as one of our sins from the past.

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

…ontainer Signed-off-by: Manel Melaouhi <[email protected]>

…the remove with acquire a new one Signed-off-by: Manel Melaouhi <[email protected]>

…roy and cleanup Signed-off-by: Manel Melaouhi <[email protected]>

Signed-off-by: Manel Melaouhi <[email protected]>

JimmyBch · 2021-02-11T12:52:06Z

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) 2019, 2021 by Robert Bosch GmbH, Apex.AI Inc. All rights reserved.
+// Copyright (c) 2019, 2021 by Robert Bosch GmbH. All rights reserved.


I guess you should add back Apex company.

Signed-off-by: Manel Melaouhi <[email protected]>

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

Signed-off-by: Manel Melaouhi <[email protected]>

…l lambda. Signed-off-by: Manel Melaouhi <[email protected]>

…lambda Signed-off-by: Manel Melaouhi <[email protected]>

…al lambda Signed-off-by: Manel Melaouhi <[email protected]>

manel94 · 2021-02-18T09:49:46Z

requested changes are pushed :D

elBoberido

The code is mostly good. Most of the requests are regarding code style and most of that is due to the existing code.

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

Signed-off-by: Manel Melaouhi <[email protected]>

…marks Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido

almost there

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp

Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido

Looks good. Thank's for the contribution

Review findings are integrated

…-Enhancing-coverage-for-class-roudi-and-port-manager-in-roudi-folder Iox eclipse-iceoryx#454 enhancing coverage for class roudi and port manager in roudi folder

JimmyBch and others added 8 commits January 28, 2021 18:53

iox-eclipse-iceoryx#454 create test file for class roudi

d8e4f8a

Signed-off-by: Jimmy Belloche <[email protected]>

iox-eclipse-iceoryx#454 clang format test file

ea8b735

Signed-off-by: Jimmy Belloche <[email protected]>

iox-eclipse-iceoryx#454 Add unit tests to improve coverage for class …

c169306

…roudi_portmanager Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 Add unit test ServiceRegistryChangeCounter an…

bb1fdb3

…d clean for class roudi_portmanager Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 merge improve-coverage-of-unit-test-in-folder…

a71cc4b

…-roudi Signed-off-by: Jimmy Belloche <[email protected]>

iox-eclipse-iceoryx#454 Change test names and use uint32_t

8e6b7d7

Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 Use EXPECT_EQ and EXPECT_NE in place of EXPEC…

a5a0274

…T_THAT Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 Define process name and node name in test base

06a1fb3

Signed-off-by: Manel Melaouhi <[email protected]>

JimmyBch reviewed Jan 29, 2021

View reviewed changes

dkroenke self-requested a review January 29, 2021 08:19

dkroenke added the test A module/integration/stress/etc test for a component label Jan 29, 2021

dkroenke reviewed Jan 29, 2021

View reviewed changes

elfenpiff requested a review from marthtz January 29, 2021 08:52

elfenpiff previously requested changes Jan 29, 2021

View reviewed changes

elBoberido reviewed Feb 1, 2021

View reviewed changes

JimmyBch and others added 6 commits February 4, 2021 11:37

iox-eclipse-iceoryx#454 improve unit test for class roudi

3c914ec

Signed-off-by: Jimmy Belloche <[email protected]>

iox-eclipse-iceoryx#454 improve unit test for class roudi

3030e11

Signed-off-by: Jimmy Belloche <[email protected]>

iox-eclipse-iceoryx#454 refactoring unit test for class roudi port ma…

bcce50d

…nager Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 ASSERT_NE in place of EXPECT_NE before pushin…

83131d3

…g to the container Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 Update port manager test file with some comme…

de378c6

…nts and replace EXPECT_* with ASSERT_* for two tests Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 remove unit test file class roudi as refactor…

8388ea1

…ing needed Signed-off-by: Jimmy Belloche <[email protected]>

Merge remote-tracking branch 'upstream/master' into iox-eclipse-iceor…

5cbd88e

…yx#454-Enhancing-coverage-for-class-roudi-and-port-manager-in-roudi-folder Signed-off-by: Chiranjeevi Maddi (RBEI/EBB1) <[email protected]>

chiranjeevimaddi force-pushed the iox-#454-Enhancing-coverage-for-class-roudi-and-port-manager-in-roudi-folder branch from 29836ed to 5cbd88e Compare February 4, 2021 13:41

elBoberido reviewed Feb 5, 2021

View reviewed changes

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp Outdated Show resolved Hide resolved

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp Outdated Show resolved Hide resolved

iceoryx_posh/test/moduletests/test_roudi_portmanager.cpp Outdated Show resolved Hide resolved

manel94 added 3 commits February 11, 2021 18:05

iox-eclipse-iceoryx#454 add template for set Destroy Flag And Clear C…

c17b337

…ontainer Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 split into two tests, the overflow check and …

180a32e

…the remove with acquire a new one Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 split destroy ports test into two tests, dest…

389e11c

…roy and cleanup Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#496 update license header

a842d87

Signed-off-by: Manel Melaouhi <[email protected]>

JimmyBch reviewed Feb 11, 2021

View reviewed changes

iox-eclipse-iceoryx#496 update copyright.

7668ea5

Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido reviewed Feb 11, 2021

View reviewed changes

mossmaurice assigned manel94 Feb 12, 2021

elBoberido mentioned this pull request Feb 15, 2021

Iox #350 implement active call set #548

Merged

19 tasks

manel94 added 4 commits February 17, 2021 11:12

iox-eclipse-iceoryx#454 fix review findings

df2982b

Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 add method exhaustInterfacePorts with optiona…

64c5984

…l lambda. Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 add method exhaustCondVarPorts with optional …

ea140ae

…lambda Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 add method exhaustNodePorts with three option…

ddef67f

…al lambda Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido reviewed Feb 19, 2021

View reviewed changes

manel94 added 2 commits February 22, 2021 11:23

iox-eclipse-iceoryx#454 fix review findings to improve code style

09f62c1

Signed-off-by: Manel Melaouhi <[email protected]>

iox-eclipse-iceoryx#454 update test cases after fixing some review re…

b188b73

…marks Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido reviewed Feb 22, 2021

View reviewed changes

iox-eclipse-iceoryx#454 update naming and delete extra variables

c6438e1

Signed-off-by: Manel Melaouhi <[email protected]>

elBoberido approved these changes Feb 23, 2021

View reviewed changes

dkroenke self-requested a review February 23, 2021 12:48

dkroenke approved these changes Feb 23, 2021

View reviewed changes

elBoberido merged commit 05ca913 into eclipse-iceoryx:master Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #454 enhancing coverage for class roudi and port manager in roudi folder #527

Iox #454 enhancing coverage for class roudi and port manager in roudi folder #527

chiranjeevimaddi commented Jan 29, 2021 •

edited by dkroenke

Loading

JimmyBch Jan 29, 2021

dkroenke Jan 29, 2021

JimmyBch Jan 29, 2021

JimmyBch Feb 1, 2021

dkroenke Feb 1, 2021

elBoberido Feb 1, 2021

JimmyBch Feb 1, 2021

elBoberido Feb 1, 2021

dkroenke left a comment

dkroenke Jan 29, 2021

dkroenke Jan 29, 2021

dkroenke Jan 29, 2021

elfenpiff left a comment

JimmyBch commented Jan 29, 2021

elBoberido left a comment

chiranjeevimaddi commented Feb 4, 2021

manel94 commented Feb 4, 2021 •

edited

Loading

elBoberido left a comment

JimmyBch Feb 11, 2021

manel94 commented Feb 18, 2021

elBoberido left a comment

elBoberido left a comment

elBoberido left a comment

		@@ -0,0 +1,545 @@
		// Copyright (c) 2021 by Robert Bosch GmbH, Apex.AI Inc. All rights reserved.

		@@ -1,4 +1,4 @@
		// Copyright (c) 2019, 2021 by Robert Bosch GmbH, Apex.AI Inc. All rights reserved.
		// Copyright (c) 2019, 2021 by Robert Bosch GmbH. All rights reserved.

Iox #454 enhancing coverage for class roudi and port manager in roudi folder #527

Iox #454 enhancing coverage for class roudi and port manager in roudi folder #527

Conversation

chiranjeevimaddi commented Jan 29, 2021 • edited by dkroenke Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

Post-review Checklist for the Eclipse Committer

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkroenke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elfenpiff left a comment

Choose a reason for hiding this comment

JimmyBch commented Jan 29, 2021

elBoberido left a comment

Choose a reason for hiding this comment

chiranjeevimaddi commented Feb 4, 2021

manel94 commented Feb 4, 2021 • edited Loading

elBoberido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manel94 commented Feb 18, 2021

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

chiranjeevimaddi commented Jan 29, 2021 •

edited by dkroenke

Loading

manel94 commented Feb 4, 2021 •

edited

Loading