jsedding commented on a change in pull request #5:
URL: 
https://github.com/apache/sling-org-apache-sling-junit-core/pull/5#discussion_r528819333



##########
File path: src/main/java/org/apache/sling/junit/impl/AnnotationsProcessor.java
##########
@@ -64,25 +67,32 @@ private void processTestReference(Object testObject, Field 
f) throws Exception {
             log.error(msg);
             throw new IllegalArgumentException(msg);
         }
-        
         final Class<?> serviceType = f.getType();
-        final Object service = getService(serviceType);
-        if(service != null) {
-            f.setAccessible(true);
-            f.set(testObject, service);
-            log.debug("Injected service {} into field {}", 
-                    serviceType.getName(), f.getName());
-        } else {
-            log.warn("Service {} not found for field {}", 
-                    serviceType.getName(), f.getName());
+        Annotation[] testReferences = f.getDeclaredAnnotations();
+        if(Objects.nonNull(testReferences) && testReferences.length != 0){
+            TestReference testReference = (TestReference) testReferences[0];
+            String filter = testReference.filter();
+            final Object service = getService(serviceType, filter);
+            if(service != null) {
+                f.setAccessible(true);
+                f.set(testObject, service);
+                log.debug("Injected service {} into field {}",
+                        serviceType.getName(), f.getName());
+            } else {
+                log.warn("Service {} not found for field {}",
+                        serviceType.getName(), f.getName());
+            }
         }
     }
-    
-    private Object getService(Class<?> c) {
+
+    private Object getService(Class<?> c, String filter) {

Review comment:
       I just realized - and this is independent of whether `ServiceGetter` is 
used or not, that OSGi mandates that services are 'ungotten' (i.e. 
BundleContext#ungetService). Otherwise we probably create a memory leak. That's 
the reason `ServiceGetter` has a `#close()` method.
   
   This is a bug predates your PR, but it would be good to resolve it while 
we're looking at it.
   
   After a brief look at the code, I believe you would need to enhance 
`SlingAnnotationsTestRunner` to provide some sort of after-test hook, robust 
enough to survive failed tests and exceptions being thrown. Maybe overriding 
`BlockJUnit4ClassRunner#run` and surrounding the call to `super.run(...)` in a 
try/finally block would work, where you give all `TestProcessor`s a chance to 
postprocess or cleanup test instances. Then you could collect all 
`ServiceGetter` instances for a test and call their `#close()` methods once the 
test instance has run.
   
   Feel free to ask for clarification if any of this is unclear!




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


Reply via email to