jsedding commented on a change in pull request #5:
URL:
https://github.com/apache/sling-org-apache-sling-junit-core/pull/5#discussion_r534424555
##########
File path: src/main/java/org/apache/sling/junit/impl/AnnotationsProcessor.java
##########
@@ -48,19 +51,30 @@ protected void activate(ComponentContext ctx) {
protected void deactivate(ComponentContext ctx) {
bundleContext = null;
log.debug("{} deactivated", this);
+ map.clear();
Review comment:
You should close all contained `ServiceGetter`s before clearing.
##########
File path:
src/main/java/org/apache/sling/junit/annotations/SlingAnnotationsTestRunner.java
##########
@@ -54,19 +60,23 @@ protected Object createTest() throws Exception {
return super.createTest();
} else {
log.debug("Using TestObjectProcessor {}", top);
- return top.process(super.createTest());
+ Object test = top.process(super.createTest());
+ tests.add(test);
Review comment:
Is a list really necessary here? I would have assumed that one runner
instance creates and runs one test at a time. But I may well be wrong. If it
*is* only one at a time, then a simple field would be enough.
##########
File path: src/main/java/org/apache/sling/junit/impl/AnnotationsProcessor.java
##########
@@ -48,19 +51,30 @@ protected void activate(ComponentContext ctx) {
protected void deactivate(ComponentContext ctx) {
bundleContext = null;
log.debug("{} deactivated", this);
+ map.clear();
}
/** Process annotations on the test object */
+ @Override
public Object process(Object testObject) throws Exception {
log.debug("processing {}", testObject);
+ map.put(testObject, new ArrayList<>());
for(Field f : testObject.getClass().getDeclaredFields()) {
if(f.isAnnotationPresent(TestReference.class)) {
processTestReference(testObject, f);
}
}
return testObject;
}
-
+
+ public void cleanupTest(Object test) {
+ List<ServiceGetter<?>> serviceGetters = this.map.get(test);
Review comment:
Instead of `this.map.get(test)` you could directly write
`this.map.remove(test)`. It returns the previously assigned value.
##########
File path:
src/main/java/org/apache/sling/junit/annotations/SlingAnnotationsTestRunner.java
##########
@@ -54,19 +60,23 @@ protected Object createTest() throws Exception {
return super.createTest();
} else {
log.debug("Using TestObjectProcessor {}", top);
- return top.process(super.createTest());
+ Object test = top.process(super.createTest());
+ tests.add(test);
+ return test;
}
}
@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();
+ AnnotationsProcessor ap = (AnnotationsProcessor) this.top;
Review comment:
Given that there are probably no other implementations out there, and it
is feasible to achieve the same with `WeakReference` + `ReferenceQueue`, I am
ok with casting here. An `instanceof` check wouldn't hurt though, just in case
someone dos register another `TestObjectProcessor`.
----------------------------------------------------------------
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
