jsedding commented on a change in pull request #5:
URL:
https://github.com/apache/sling-org-apache-sling-junit-core/pull/5#discussion_r529609030
##########
File path:
src/main/java/org/apache/sling/junit/annotations/SlingAnnotationsTestRunner.java
##########
@@ -56,4 +57,16 @@ protected Object createTest() throws Exception {
return top.process(super.createTest());
}
}
+
+ @Override
+ public void run(RunNotifier notifier){
+ try {
+ super.run(notifier);
+ } catch (Exception e) {
+ log.error("Test 'run' method", e);
+ } finally {
+ AnnotationsProcessor ap = (AnnotationsProcessor) top;
+ ap.closeAllServices();
Review comment:
I think this is not what you want to do here. If I understand the code
correctly, `AnnotationsProcessor` implements `TestObjectProcessor` and is a
service, which in turn implies it is a singleton. On the other hand, there is
probably an instance of `SlingAnnotationsTestRunner` per test class. The
current code, if I read it correctly, closes all services of all tests. This
may not be a practical issue as long as tests are not executed concurrently,
but it is still not correct.
The `ServiceGetter` instances are created for a specific test, namely the
one created by the call `top.process(super.createTest());` above. I would add a
method to `TestObjectProcessor` that allows cleaning up after a test. I'll
provisionally call it `TestObjectProcessor#cleanupTest(Object test)`. Then
instead of keeping an instance of `TestObjectProcessor` in a field, I would
keep an instance of the `Object` returned by `super.createTest()` in a field.
In the finally block, this object can then be passed to the cleanup method.
In `AnnotationsProcessor` you keep an `IdentityHashMap<Object,
List<ServiceGetter>>`, which you initialize in the `#process(Object)` method.
Finally, in the `#cleanup(Object)` method, you close all `ServiceGetter`s
pertaining to the `object` and remove it from the map. Don't forget to clear
the map on `@Deactivate` of `AnnotationsProcessor`.
You may want to provide a no-op default implementation of `#cleanup(Object)`
in the interface `TestObjectProcessor`. This might (I'm not sure it does)
reduce the required version increase, and it would certainly keep any
implementations out in the wild running without code changes (if they indeed
exist... who knows).
ALTERNATIVE: If you're feeling adventurous and have some time, there would
be an alternative to adding a new method to the API. You could use
`WeakReference` and `ReferenceQueue` with a cleanup thread inside
`AnnotationsProcessor` in order to close the `ServiceGetter`s. The approach is
[nicely summed up on stack
overflow](https://stackoverflow.com/questions/14450538/using-javas-referencequeue/14450693#14450693),
where `Foo` would be the test `Object` and `Bar` the `List<ServiceGetter>`.
##########
File path: src/main/java/org/apache/sling/junit/impl/AnnotationsProcessor.java
##########
@@ -35,9 +34,11 @@
public class AnnotationsProcessor implements TestObjectProcessor {
private Logger log = LoggerFactory.getLogger(getClass());
private BundleContext bundleContext;
-
+ private ArrayList<ServiceGetter<? extends Object>> serviceGetters;
Review comment:
I prefer using interface types for all fields rather than
implementations. That makes refactoring to a different type easier and the diff
less noisy.
##########
File path:
src/main/java/org/apache/sling/junit/annotations/SlingAnnotationsTestRunner.java
##########
@@ -56,4 +57,16 @@ protected Object createTest() throws Exception {
return top.process(super.createTest());
}
}
+
+ @Override
+ public void run(RunNotifier notifier){
+ try {
+ super.run(notifier);
+ } catch (Exception e) {
+ log.error("Test 'run' method", e);
Review comment:
Don't catch the exception here. You are decorating, so ideally the
calling code gets the same result as without the decoration (except for desired
changes/side-effects). In case my previous comments prompted you to add the
catch block, I'm sorry for causing confusion. The finally block is sufficient,
because it gets called in any case, exception or not.
----------------------------------------------------------------
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:
us...@infra.apache.org
