[GitHub] [celix] Oipo commented on a change in pull request #293: Feature/add build to svc dep creation

[GitBox <gi...@apache.org>]

Oipo commented on a change in pull request #293:
URL: https://github.com/apache/celix/pull/293#discussion_r521640934



##########
File path: libs/framework/gtest/src/dm_tests.cpp
##########
@@ -75,3 +75,113 @@ TEST_F(DepenencyManagerTests, TestCheckActive) {
     celix_dependencyManager_add(mng, cmp);
     ASSERT_FALSE(celix_dependencyManager_areComponentsActive(mng));
 }
+
+class TestComponent {
+
+};
+
+TEST_F(DepenencyManagerTests, OnlyActiveAfterBuildCheck) {
+    celix::dm::DependencyManager dm{ctx};
+    EXPECT_EQ(0, dm.getNrOfComponents());
+
+    auto& cmp = dm.createComponent<TestComponent>(std::make_shared<TestComponent>(), "test1");
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm not started yet / comp not build yet
+    EXPECT_TRUE(cmp.isValid());
+
+    cmp.build();
+    cmp.build(); //should be ok to call twice

Review comment:
       :+1: nice tests

##########
File path: libs/framework/include/celix/dm/Component.h
##########
@@ -52,6 +49,22 @@ namespace celix { namespace dm {
          * Returns the C bundle context
          */
         celix_bundle_context_t* bundleContext() const { return this->context; }
+
+        void runBuild();
+    protected:
+        std::vector<std::shared_ptr<BaseServiceDependency>> dependencies{};
+
+        // 0 = service name

Review comment:
       Perhaps worth considering just making a simple struct with names for the variables, rather than tuple. Then these comments are not needed anymore :)

##########
File path: libs/framework/gtest/src/dm_tests.cpp
##########
@@ -75,3 +75,113 @@ TEST_F(DepenencyManagerTests, TestCheckActive) {
     celix_dependencyManager_add(mng, cmp);
     ASSERT_FALSE(celix_dependencyManager_areComponentsActive(mng));
 }
+
+class TestComponent {
+
+};
+
+TEST_F(DepenencyManagerTests, OnlyActiveAfterBuildCheck) {

Review comment:
       Can you rename `DepenencyManagerTests` to `DependencyManagerTests`?

##########
File path: libs/framework/include/celix/dm/Component.h
##########
@@ -243,9 +255,19 @@ namespace celix { namespace dm {
          * @return the DM Component reference for chaining (fluent API)
          */
         Component<T>& removeCallbacks();
+
+        /**
+         * Build the component.
+         *
+         * When building the component all provided services and services dependencies are enabled.
+         * This is not done automatically so that user can firs construct component with their provided

Review comment:
       `firs` -> `first`

##########
File path: libs/framework/include/celix/dm/Component_Impl.h
##########
@@ -63,7 +88,7 @@ Component<T>& Component<T>::addInterfaceWithName(const std::string &serviceName,
 
 template<class T>
 template<class I>
-Component<T>& Component<T>::addInterface(const std::string &version, const Properties &properties) {
+inline Component<T>& Component<T>::addInterface(const std::string &version, const Properties &properties) {

Review comment:
       I'm generally not in favour of using the inline keyword, the compiler is free to ignore it and usually knows better than programmers anyway. Moreover, templated functions in headers are already inline by default.

##########
File path: libs/framework/include/celix/dm/ServiceDependency.h
##########
@@ -219,13 +229,13 @@ namespace celix { namespace dm {
     class ServiceDependency : public TypedServiceDependency<T> {
         using type = I;
     public:
-        ServiceDependency(const std::string &name = std::string{}, bool valid = true);
+        ServiceDependency(celix_dm_component_t* cCmp, const std::string &name, bool valid = true);
         ~ServiceDependency() override = default;
 
         ServiceDependency(const ServiceDependency&) = delete;
         ServiceDependency& operator=(const ServiceDependency&) = delete;
-        ServiceDependency(ServiceDependency&&) noexcept = default;
-        ServiceDependency& operator=(ServiceDependency&&) noexcept = default;
+        ServiceDependency(ServiceDependency&&) noexcept = delete;
+        ServiceDependency& operator=(ServiceDependency&&) noexcept = delete;

Review comment:
       It's late so maybe I'm missing something, but semantically, I would expect moving of all these service dependencies to be OK. What's the reason you put it on delete?

##########
File path: libs/framework/gtest/src/dm_tests.cpp
##########
@@ -75,3 +75,113 @@ TEST_F(DepenencyManagerTests, TestCheckActive) {
     celix_dependencyManager_add(mng, cmp);
     ASSERT_FALSE(celix_dependencyManager_areComponentsActive(mng));
 }
+
+class TestComponent {
+
+};
+
+TEST_F(DepenencyManagerTests, OnlyActiveAfterBuildCheck) {
+    celix::dm::DependencyManager dm{ctx};
+    EXPECT_EQ(0, dm.getNrOfComponents());
+
+    auto& cmp = dm.createComponent<TestComponent>(std::make_shared<TestComponent>(), "test1");
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm not started yet / comp not build yet
+    EXPECT_TRUE(cmp.isValid());
+
+    cmp.build();
+    cmp.build(); //should be ok to call twice
+    EXPECT_EQ(1, dm.getNrOfComponents()); //cmp "build", so active
+
+    dm.clear();
+    dm.clear(); //should be ok to call twice
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm cleared so no components
+}
+
+TEST_F(DepenencyManagerTests, StartDmWillBuildCmp) {
+    celix::dm::DependencyManager dm{ctx};
+    EXPECT_EQ(0, dm.getNrOfComponents());
+
+    auto& cmp = dm.createComponent<TestComponent>(std::make_shared<TestComponent>(), "test1");
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm not started yet / comp not build yet
+    EXPECT_TRUE(cmp.isValid());
+
+    dm.start();
+    EXPECT_EQ(1, dm.getNrOfComponents()); //cmp "build", so active
+
+    dm.stop();
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm cleared so no components
+}
+
+struct TestService {
+    void *handle;
+};
+
+TEST_F(DepenencyManagerTests, AddSvcProvideAfterBuild) {
+    celix::dm::DependencyManager dm{ctx};
+    EXPECT_EQ(0, dm.getNrOfComponents());
+
+    auto& cmp = dm.createComponent<TestComponent>(std::make_shared<TestComponent>(), "test1");
+    EXPECT_EQ(0, dm.getNrOfComponents()); //dm not started yet / comp not build yet
+    EXPECT_TRUE(cmp.isValid());
+
+    cmp.build();
+    EXPECT_EQ(1, dm.getNrOfComponents()); //cmp "build", so active
+
+    TestService svc{nullptr};
+    cmp.addCInterface(&svc, "TestService");
+
+    long svcId = celix_bundleContext_findService(ctx, "TestService");
+    EXPECT_EQ(-1, svcId); //not build -> not found
+
+    cmp.build();
+    cmp.build(); //should be ok to call twice
+    svcId = celix_bundleContext_findService(ctx, "TestService");
+    EXPECT_GE(svcId, -1); //(re)build -> found

Review comment:
       `GE 0` or `GT -1`, I think you meant here.




----------------------------------------------------------------
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