You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2021/03/18 18:29:30 UTC

[GitHub] [celix] pnoltes opened a new pull request #332: Adds some missing sync calls.

pnoltes opened a new pull request #332:
URL: https://github.com/apache/celix/pull/332


   Also adds the option to add a component context object.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] codecov-io commented on pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #332:
URL: https://github.com/apache/celix/pull/332#issuecomment-806619631


   # [Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=h1) Report
   > Merging [#332](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=desc) (766af1a) into [master](https://codecov.io/gh/apache/celix/commit/0729c4dfaa6b45f436e41f13ebe71960c746d109?el=desc) (0729c4d) will **increase** coverage by `0.02%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/celix/pull/332/graphs/tree.svg?width=650&height=150&src=pr&token=JdsiThga8P)](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #332      +/-   ##
   ==========================================
   + Coverage   70.96%   70.99%   +0.02%     
   ==========================================
     Files         175      175              
     Lines       31570    31598      +28     
   ==========================================
   + Hits        22405    22434      +29     
   + Misses       9165     9164       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libs/framework/include/celix/dm/Component.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnQuaA==) | `81.81% <ø> (ø)` | |
   | [...ibs/framework/include/celix/dm/DependencyManager.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlci5o) | `100.00% <ø> (ø)` | |
   | [libs/framework/include/celix\_bundle\_context.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeF9idW5kbGVfY29udGV4dC5o) | `100.00% <ø> (ø)` | |
   | [libs/framework/include/celix/BundleActivator.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9CdW5kbGVBY3RpdmF0b3IuaA==) | `69.69% <100.00%> (ø)` | |
   | [libs/framework/include/celix/BundleContext.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9CdW5kbGVDb250ZXh0Lmg=) | `100.00% <100.00%> (+0.96%)` | :arrow_up: |
   | [libs/framework/include/celix/FrameworkFactory.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9GcmFtZXdvcmtGYWN0b3J5Lmg=) | `100.00% <100.00%> (ø)` | |
   | [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.44% <100.00%> (+0.38%)` | :arrow_up: |
   | [...ramework/include/celix/dm/DependencyManager\_Impl.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlcl9JbXBsLmg=) | `99.14% <100.00%> (+0.08%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=footer). Last update [0729c4d...766af1a](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #332:
URL: https://github.com/apache/celix/pull/332#discussion_r601151012



##########
File path: libs/framework/gtest/src/DependencyManagerTestSuite.cc
##########
@@ -638,4 +639,12 @@ TEST_F(DependencyManagerTestSuite, ExceptionsInLifecycle) {
         //required service -> should stop, but fails at stop and should become inactive (component will disable itself)
         dm.clear(); //dm clear will deinit component and this should fail, but not deadlock
     }
+}
+
+TEST_F(DependencyManagerTestSuite, ComponentContext) {

Review comment:
       same

##########
File path: libs/framework/gtest/src/CxxBundleContextTestSuite.cc
##########
@@ -561,3 +565,21 @@ TEST_F(CxxBundleContextTestSuite, setServicesWithTrackerWhenMultipleRegistration
     //Also look into why this is not happening in the C service tracker test.
     EXPECT_EQ(3, count.load());
 }
+
+TEST_F(CxxBundleContextTestSuite, WaitForAllEvents) {
+    long svcId = ctx->findService<TestInterface>();
+    EXPECT_EQ(svcId, -1L);
+
+    {
+        auto impl = std::make_shared<TestImplementation>();
+        auto svcReg = ctx->registerService<TestInterface>(impl).build();
+        svcReg->wait();
+        svcId = ctx->findService<TestInterface>();
+        EXPECT_GE(svcId, 0L);
+    }
+
+    ctx->waitForAllEvents();
+    ctx->waitIfAbleForAllEvents();

Review comment:
       same




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] codecov-io edited a comment on pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #332:
URL: https://github.com/apache/celix/pull/332#issuecomment-806619631


   # [Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=h1) Report
   > Merging [#332](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=desc) (766af1a) into [master](https://codecov.io/gh/apache/celix/commit/0729c4dfaa6b45f436e41f13ebe71960c746d109?el=desc) (0729c4d) will **increase** coverage by `0.04%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/celix/pull/332/graphs/tree.svg?width=650&height=150&src=pr&token=JdsiThga8P)](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #332      +/-   ##
   ==========================================
   + Coverage   70.96%   71.01%   +0.04%     
   ==========================================
     Files         175      175              
     Lines       31570    31598      +28     
   ==========================================
   + Hits        22405    22438      +33     
   + Misses       9165     9160       -5     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [libs/framework/include/celix/dm/Component.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnQuaA==) | `81.81% <ø> (ø)` | |
   | [...ibs/framework/include/celix/dm/DependencyManager.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlci5o) | `100.00% <ø> (ø)` | |
   | [libs/framework/include/celix\_bundle\_context.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeF9idW5kbGVfY29udGV4dC5o) | `100.00% <ø> (ø)` | |
   | [libs/framework/include/celix/BundleActivator.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9CdW5kbGVBY3RpdmF0b3IuaA==) | `69.69% <100.00%> (ø)` | |
   | [libs/framework/include/celix/BundleContext.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9CdW5kbGVDb250ZXh0Lmg=) | `100.00% <100.00%> (+0.96%)` | :arrow_up: |
   | [libs/framework/include/celix/FrameworkFactory.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9GcmFtZXdvcmtGYWN0b3J5Lmg=) | `100.00% <100.00%> (ø)` | |
   | [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.44% <100.00%> (+0.38%)` | :arrow_up: |
   | [...ramework/include/celix/dm/DependencyManager\_Impl.h](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9EZXBlbmRlbmN5TWFuYWdlcl9JbXBsLmg=) | `99.14% <100.00%> (+0.08%)` | :arrow_up: |
   | [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/332/diff?src=pr&el=tree#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `91.85% <0.00%> (+1.12%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=footer). Last update [0729c4d...766af1a](https://codecov.io/gh/apache/celix/pull/332?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #332:
URL: https://github.com/apache/celix/pull/332#discussion_r601150676



##########
File path: libs/framework/gtest/src/CxxBundleContextTestSuite.cc
##########
@@ -78,6 +78,7 @@ TEST_F(CxxBundleContextTestSuite, RegisterServiceTest) {
     }
 
     ctx->waitForEvents();
+    ctx->waitIfAbleForEvents();

Review comment:
       yes, and will remote the superfluous wait entries. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes merged pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #332:
URL: https://github.com/apache/celix/pull/332


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] Oipo commented on a change in pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #332:
URL: https://github.com/apache/celix/pull/332#discussion_r597568117



##########
File path: libs/framework/gtest/src/CxxBundleContextTestSuite.cc
##########
@@ -78,6 +78,7 @@ TEST_F(CxxBundleContextTestSuite, RegisterServiceTest) {
     }
 
     ctx->waitForEvents();
+    ctx->waitIfAbleForEvents();

Review comment:
       Did you forget to remove line 80?

##########
File path: libs/framework/gtest/src/DependencyManagerTestSuite.cc
##########
@@ -638,4 +639,12 @@ TEST_F(DependencyManagerTestSuite, ExceptionsInLifecycle) {
         //required service -> should stop, but fails at stop and should become inactive (component will disable itself)
         dm.clear(); //dm clear will deinit component and this should fail, but not deadlock
     }
+}
+
+TEST_F(DependencyManagerTestSuite, ComponentContext) {

Review comment:
       Please rename `ComponentContext` to `ComponentContextShouldNotLeak`.

##########
File path: libs/framework/gtest/src/CxxBundleContextTestSuite.cc
##########
@@ -561,3 +565,21 @@ TEST_F(CxxBundleContextTestSuite, setServicesWithTrackerWhenMultipleRegistration
     //Also look into why this is not happening in the C service tracker test.
     EXPECT_EQ(3, count.load());
 }
+
+TEST_F(CxxBundleContextTestSuite, WaitForAllEvents) {
+    long svcId = ctx->findService<TestInterface>();
+    EXPECT_EQ(svcId, -1L);
+
+    {
+        auto impl = std::make_shared<TestImplementation>();
+        auto svcReg = ctx->registerService<TestInterface>(impl).build();
+        svcReg->wait();
+        svcId = ctx->findService<TestInterface>();
+        EXPECT_GE(svcId, 0L);
+    }
+
+    ctx->waitForAllEvents();
+    ctx->waitIfAbleForAllEvents();

Review comment:
       Again a double call here that doesn't look like it adds anything. Copy and paste error or am I missing something?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [celix] pnoltes commented on a change in pull request #332: Adds some missing async calls.

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #332:
URL: https://github.com/apache/celix/pull/332#discussion_r601151012



##########
File path: libs/framework/gtest/src/DependencyManagerTestSuite.cc
##########
@@ -638,4 +639,12 @@ TEST_F(DependencyManagerTestSuite, ExceptionsInLifecycle) {
         //required service -> should stop, but fails at stop and should become inactive (component will disable itself)
         dm.clear(); //dm clear will deinit component and this should fail, but not deadlock
     }
+}
+
+TEST_F(DependencyManagerTestSuite, ComponentContext) {

Review comment:
       updated




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org