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:
[email protected]
