celix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [celix] Oipo commented on a change in pull request #293: Feature/add build to svc dep creation
Date Wed, 11 Nov 2020 21:33:08 GMT

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



Mime
View raw message