Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/778#discussion_r73416186
--- Diff:
nifi-nar-bundles/nifi-standard-services/nifi-dbcp-service-bundle/nifi-dbcp-service/src/test/java/org/apache/nifi/dbcp/DBCPServiceTest.java
---
@@ -258,6 +264,28 @@ public void testURLClassLoaderGetConnection() throws
ClassNotFoundException, Mal
DriverManager.deregisterDriver(shim);
}
+ @Test
+ public void testPropertiesUseVariableRegistry() throws
InitializationException, SQLException {
+ final VariableRegistry variableRegistry =
mock(VariableRegistry.class);
+
given(variableRegistry.getVariableValue("databaseurl")).willReturn("jdbc:derby:"
+ DB_LOCATION + ";create=true");
+
given(variableRegistry.getVariableValue("drivername")).willReturn("org.apache.derby.jdbc.EmbeddedDriver");
+
given(variableRegistry.getVariableValue("username")).willReturn("tester");
+
given(variableRegistry.getVariableValue("password")).willReturn("testerp");
+
+ final TestRunner runner =
TestRunners.newTestRunner(mock(org.apache.nifi.processor.Processor.class),
variableRegistry);
+ final DBCPConnectionPool service = new DBCPConnectionPool();
+ runner.addControllerService("dbcpService", service);
+
+ runner.setProperty(service, DBCPConnectionPool.DATABASE_URL,
"${databaseurl}");
+ runner.setProperty(service, DBCPConnectionPool.DB_DRIVERNAME,
"${drivername}");
+ runner.setProperty(service, DBCPConnectionPool.DB_USER,
"${username}");
+ runner.setProperty(service, DBCPConnectionPool.DB_PASSWORD,
"${password}");
+
+ runner.enableControllerService(service);
+
+
then(variableRegistry).should(times(4)).getVariableValue(anyString());
--- End diff --
This feels brittle. The controller service being tested does not call into
the variable registry - it simply calls evaluateAttributeExpressions(). So
verifying that the mock framework calls into the variable registry 4 times is
really testing the mock framework moreso than the Controller Service. The
Controller Service (or mock framework) could certainly be changed so that they
call getVariableValue() many more times without any change in functionality, as
well.
I think a better test would be to ensure that each time that the property's
value is obtained, the EL is first evaluated. This is already done by the mock
framework - if the PropertyDescriptor indicates that it supports expression
language but getValue() is called on the PropertyValue without first calling
evaluateAttributeExpressions(), the mock framework calls Assert.fail()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
