Reapply "Make it possible to test the Container with synthetic bundles"

This reverts commit cbed8a40c2a72ca09b7b8e97f83c3acde3479ab3.

7 files changed, 183 insertions, 120 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/di/Container.java b/container-core/src/main/java/com/yahoo/container/di/Container.java
index e45a5bf2ac5..db463edecc0 100644
--- a/container-core/src/main/java/com/yahoo/container/di/Container.java
+++ b/container-core/src/main/java/com/yahoo/container/di/Container.java
@@ -67,11 +67,6 @@ public class Container {
         this.retriever = new ConfigRetriever(bootstrapKeys, subscriberFactory);
     }
 
-    public Container(SubscriberFactory subscriberFactory, String configId, ComponentDeconstructor destructor) {
-        this(subscriberFactory, configId, destructor, new Osgi() {
-        });
-    }
-
     // TODO: try to simplify by returning the result even when the graph failed, instead of throwing here.
     public ComponentGraphResult waitForNextGraphGeneration(ComponentGraph oldGraph, Injector fallbackInjector, boolean isInitializing) {
         try {
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
index 04ed52db53b..bc5b6d6069b 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
@@ -1,19 +1,19 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.core.config;
 
-import com.yahoo.config.FileReference;
-import com.yahoo.filedistribution.fileacquirer.FileAcquirer;
-import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer;
-import com.yahoo.osgi.Osgi;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.osgi.framework.Bundle;
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_1;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_1_REF;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_2;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_2_REF;
+import static com.yahoo.container.core.config.BundleTestUtil.testBundles;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -22,20 +22,13 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
  */
 public class ApplicationBundleLoaderTest {
 
-    private static final FileReference BUNDLE_1_REF = new FileReference("bundle-1");
-    private static final Bundle BUNDLE_1 = new TestBundle(BUNDLE_1_REF.value());
-    private static final FileReference BUNDLE_2_REF = new FileReference("bundle-2");
-    private static final Bundle BUNDLE_2 = new TestBundle(BUNDLE_2_REF.value());
-
     private ApplicationBundleLoader bundleLoader;
     private TestOsgi osgi;
 
     @BeforeEach
     public void setup() {
         osgi = new TestOsgi(testBundles());
-        var bundleInstaller = new TestBundleInstaller(MockFileAcquirer.returnFile(null));
-
-        bundleLoader = new ApplicationBundleLoader(osgi, bundleInstaller);
+        bundleLoader = osgi.bundleLoader();
     }
 
     @Test
@@ -122,22 +115,4 @@ public class ApplicationBundleLoaderTest {
         assertEquals(BUNDLE_1_REF, bundleLoader.getActiveFileReferences().get(0));
     }
 
-    private static Map<String, Bundle> testBundles() {
-        return Map.of(BUNDLE_1_REF.value(), BUNDLE_1,
-                      BUNDLE_2_REF.value(), BUNDLE_2);
-    }
-
-    static class TestBundleInstaller extends FileAcquirerBundleInstaller {
-
-        TestBundleInstaller(FileAcquirer fileAcquirer) {
-            super(fileAcquirer);
-        }
-
-        @Override
-        public List<Bundle> installBundles(FileReference reference, Osgi osgi) {
-            return osgi.install(reference.value());
-        }
-
-    }
-
 }
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java b/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java
new file mode 100644
index 00000000000..fddbeaf42be
--- /dev/null
+++ b/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java
@@ -0,0 +1,39 @@
+package com.yahoo.container.core.config;
+
+import com.yahoo.config.FileReference;
+import com.yahoo.filedistribution.fileacquirer.FileAcquirer;
+import com.yahoo.osgi.Osgi;
+import org.osgi.framework.Bundle;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * @author gjoranv
+ */
+public class BundleTestUtil {
+
+    public static final FileReference BUNDLE_1_REF = new FileReference("bundle-1");
+    public static final Bundle BUNDLE_1 = new TestBundle(BUNDLE_1_REF.value());
+    public static final FileReference BUNDLE_2_REF = new FileReference("bundle-2");
+    public static final Bundle BUNDLE_2 = new TestBundle(BUNDLE_2_REF.value());
+
+    public static Map<String, Bundle> testBundles() {
+        return Map.of(BUNDLE_1_REF.value(), BUNDLE_1,
+                      BUNDLE_2_REF.value(), BUNDLE_2);
+    }
+
+    public static class TestBundleInstaller extends FileAcquirerBundleInstaller {
+
+        TestBundleInstaller(FileAcquirer fileAcquirer) {
+            super(fileAcquirer);
+        }
+
+        @Override
+        public List<Bundle> installBundles(FileReference reference, Osgi osgi) {
+            return osgi.install(reference.value());
+        }
+
+    }
+
+}
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java b/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
index 4a607520b2a..17709844f99 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
@@ -16,7 +16,7 @@ import java.util.List;
 /**
  * @author gjoranv
  */
-class TestBundle extends MockBundle {
+public class TestBundle extends MockBundle {
 
     private static final BundleRevision revision = new TestBundleRevision();
 
@@ -24,7 +24,7 @@ class TestBundle extends MockBundle {
 
     boolean started = false;
 
-    TestBundle(String symbolicName) {
+    public TestBundle(String symbolicName) {
         this.symbolicName = symbolicName;
     }
 
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java b/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
index cf3d1cef5f8..0df338c2144 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
@@ -1,6 +1,8 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.core.config;
 
+import com.yahoo.config.FileReference;
+import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer;
 import com.yahoo.osgi.MockOsgi;
 import org.osgi.framework.Bundle;
 
@@ -8,21 +10,28 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * @author gjoranv
  */
-class TestOsgi extends MockOsgi {
+public class TestOsgi extends MockOsgi implements com.yahoo.container.di.Osgi {
 
+    private final ApplicationBundleLoader bundleLoader;
     private final Map<String, Bundle> availableBundles;
 
     private final List<Bundle> installedBundles = new ArrayList<>();
     private final List<Bundle> allowedDuplicates = new ArrayList<>();
 
-    TestOsgi(Map<String, Bundle> availableBundles) {
+    public TestOsgi(Map<String, Bundle> availableBundles) {
         this.availableBundles = availableBundles;
+
+        var bundleInstaller = new BundleTestUtil.TestBundleInstaller(MockFileAcquirer.returnFile(null));
+        bundleLoader = new ApplicationBundleLoader(this, bundleInstaller);
     }
 
+    public ApplicationBundleLoader bundleLoader() { return bundleLoader; }
+
     @Override
     public List<Bundle> install(String fileReferenceValue) {
         if (! availableBundles.containsKey(fileReferenceValue))
@@ -55,4 +64,17 @@ class TestOsgi extends MockOsgi {
         allowedDuplicates.addAll(bundles);
     }
 
+    @Override
+    public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
+        return bundleLoader.useBundles(new ArrayList<>(bundles));
+    }
+
+    @Override
+    public Collection<Bundle> revertApplicationBundles() {
+        return bundleLoader.revertToPreviousGeneration();
+    }
+
+    public void removeBundle(Bundle bundle) {
+        installedBundles.remove(bundle);
+    }
 }
diff --git a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
index 04091c25ad6..ccd60390129 100644
--- a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
+++ b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di;
 
-import com.google.inject.Guice;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.config.di.IntConfig;
 import com.yahoo.config.test.TestConfig;
@@ -11,16 +10,20 @@ import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.SimpleCompo
 import com.yahoo.container.di.componentgraph.core.ComponentNode.ComponentConstructorException;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
-import org.osgi.framework.Bundle;
 
-import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @author Tony Vaagenes
@@ -88,6 +91,28 @@ public class ContainerTest extends ContainerTestBase {
         container.shutdownConfigRetriever();
     }
 
+    @Test
+    void bundle_from_previous_generation_is_uninstalled_when_not_used_in_the_new_generation() {
+        ComponentEntry component1 = new ComponentEntry("component1", SimpleComponent.class);
+        ComponentEntry component2 = new ComponentEntry("component2", SimpleComponent.class);
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-1"), List.of(component1));
+        Container container = newContainer(dirConfigSource);
+        ComponentGraph graph = getNewComponentGraph(container);
+
+        // bundle-1 is installed
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-2"), List.of(component2));
+        container.reloadConfig(2);
+        getNewComponentGraph(container, graph);
+
+        // bundle-1 is kept, bundle-2 has been uninstalled
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-2", osgi.getBundles()[0].getSymbolicName());
+    }
+
     //@Test TODO
     public void deconstructor_is_given_guice_components() {
     }
@@ -179,6 +204,34 @@ public class ContainerTest extends ContainerTestBase {
     }
 
     @Test
+    void bundle_from_failing_generation_is_uninstalled() {
+        ComponentEntry simpleComponentEntry = new ComponentEntry("simpleComponent", SimpleComponent.class);
+        ComponentEntry throwingComponentEntry = new ComponentEntry("throwingComponent", ComponentThrowingExceptionInConstructor.class);
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-1"), List.of(simpleComponentEntry));
+        Container container = newContainer(dirConfigSource);
+        ComponentGraph currentGraph = getNewComponentGraph(container);
+
+        // bundle-1 is installed
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-2"), List.of(throwingComponentEntry));
+        container.reloadConfig(2);
+        try {
+            currentGraph = getNewComponentGraph(container, currentGraph);
+            fail("Expected exception");
+        } catch (ComponentConstructorException ignored) {
+            // Expected, do nothing
+        }
+        assertEquals(1, currentGraph.generation());
+
+        // bundle-1 is kept, bundle-2 has been uninstalled
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+    }
+
+    @Test
     void getNewComponentGraph_hangs_waiting_for_valid_config_after_invalid_config() throws Exception {
         dirConfigSource.writeConfig("test", "stringVal \"original\"");
         writeBootstrapConfigs("myId", ComponentTakingConfig.class);
@@ -303,38 +356,6 @@ public class ContainerTest extends ContainerTestBase {
         }
     }
 
-    public static class TestDeconstructor implements ComponentDeconstructor {
-        @Override
-        public void deconstruct(long generation, List<Object> components, Collection<Bundle> bundles) {
-            components.forEach(component -> {
-                if (component instanceof DestructableComponent) {
-                    DestructableComponent vespaComponent = (DestructableComponent) component;
-                    vespaComponent.deconstruct();
-                }
-            });
-            if (! bundles.isEmpty()) throw new IllegalArgumentException("This test should not use bundles");
-        }
-    }
-
-    private static Container newContainer(DirConfigSource dirConfigSource,
-                                          ComponentDeconstructor deconstructor) {
-        return new Container(new CloudSubscriberFactory(dirConfigSource.configSource), dirConfigSource.configId(), deconstructor);
-    }
-
-    private static Container newContainer(DirConfigSource dirConfigSource) {
-        return newContainer(dirConfigSource, new TestDeconstructor());
-    }
-
-    ComponentGraph getNewComponentGraph(Container container, ComponentGraph oldGraph) {
-        Container.ComponentGraphResult result = container.waitForNextGraphGeneration(oldGraph, Guice.createInjector(), true);
-        result.oldComponentsCleanupTask().run();
-        return result.newGraph();
-    }
-
-    ComponentGraph getNewComponentGraph(Container container) {
-        return container.waitForNextGraphGeneration(new ComponentGraph(), Guice.createInjector(), true).newGraph();
-    }
-
     private ComponentTakingConfig createComponentTakingConfig(ComponentGraph componentGraph) {
         return componentGraph.getInstance(ComponentTakingConfig.class);
     }
diff --git a/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java b/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
index f2faed650a8..7442eb2068d 100644
--- a/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
+++ b/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
@@ -2,9 +2,8 @@
 package com.yahoo.container.di;
 
 import com.google.inject.Guice;
-import com.yahoo.component.ComponentSpecification;
-import com.yahoo.config.FileReference;
-import com.yahoo.container.bundle.BundleInstantiationSpecification;
+import com.yahoo.container.core.config.BundleTestUtil;
+import com.yahoo.container.core.config.TestOsgi;
 import com.yahoo.container.di.ContainerTest.ComponentTakingConfig;
 import com.yahoo.container.di.componentgraph.core.ComponentGraph;
 import org.junit.jupiter.api.AfterEach;
@@ -14,9 +13,7 @@ import org.osgi.framework.Bundle;
 
 import java.io.File;
 import java.util.Collection;
-import java.util.Set;
-
-import static java.util.Collections.emptySet;
+import java.util.List;
 
 /**
  * @author Tony Vaagenes
@@ -27,6 +24,7 @@ public class ContainerTestBase {
 
     private ComponentGraph componentGraph;
     protected DirConfigSource dirConfigSource = null;
+    protected TestOsgi osgi;
 
     @TempDir
     File tmpDir;
@@ -34,6 +32,8 @@ public class ContainerTestBase {
     @BeforeEach
     public void setup() {
         dirConfigSource = new DirConfigSource(tmpDir, "ContainerTest-");
+        componentGraph = new ComponentGraph(0);
+        osgi = new TestOsgi(BundleTestUtil.testBundles());
     }
 
     @AfterEach
@@ -41,52 +41,43 @@ public class ContainerTestBase {
         dirConfigSource.cleanup();
     }
 
-    @BeforeEach
-    public void createGraph() {
-        componentGraph = new ComponentGraph(0);
+
+    protected Container newContainer(DirConfigSource dirConfigSource,
+                                            ComponentDeconstructor deconstructor) {
+        return new Container(new CloudSubscriberFactory(dirConfigSource.configSource),
+                             dirConfigSource.configId(),
+                             deconstructor,
+                             osgi);
     }
 
-    public void complete() {
-        try {
-            Container container = new Container(new CloudSubscriberFactory(dirConfigSource.configSource()), dirConfigSource.configId(),
-                    new ContainerTest.TestDeconstructor(), new Osgi() {
-                        @SuppressWarnings("unchecked")
-                        @Override
-                        public Class<Object> resolveClass(BundleInstantiationSpecification spec) {
-                            try {
-                                return (Class<Object>) Class.forName(spec.classId.getName());
-                            } catch (ClassNotFoundException e) {
-                                throw new RuntimeException(e);
-                            }
-                        }
-
-                        @Override
-                        public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
-                            return emptySet();
-                        }
-
-                        @Override
-                        public Collection<Bundle> revertApplicationBundles() {
-                            return emptySet();
-                        }
-
-                        @Override
-                        public Bundle getBundle(ComponentSpecification spec) {
-                            throw new UnsupportedOperationException("getBundle not supported.");
-                        }
-                    });
-            Container.ComponentGraphResult result = container.waitForNextGraphGeneration(this.componentGraph, Guice.createInjector(), true);
-            result.oldComponentsCleanupTask().run();
-            this.componentGraph = result.newGraph();
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
+    protected Container newContainer(DirConfigSource dirConfigSource) {
+        return newContainer(dirConfigSource, new TestDeconstructor(osgi));
     }
 
+    ComponentGraph getNewComponentGraph(Container container, ComponentGraph oldGraph) {
+        Container.ComponentGraphResult result = container.waitForNextGraphGeneration(oldGraph, Guice.createInjector(), true);
+        result.oldComponentsCleanupTask().run();
+        return result.newGraph();
+    }
+
+    ComponentGraph getNewComponentGraph(Container container) {
+        return container.waitForNextGraphGeneration(new ComponentGraph(), Guice.createInjector(), true).newGraph();
+    }
+
+
     public <T> T getInstance(Class<T> componentClass) {
         return componentGraph.getInstance(componentClass);
     }
 
+    protected void writeBootstrapConfigsWithBundles(List<String> applicationBundles, List<ComponentEntry> components) {
+        writeBootstrapConfigs(components.toArray(new ComponentEntry[0]));
+        StringBuilder bundles = new StringBuilder();
+        for (int i = 0; i < applicationBundles.size(); i++) {
+            bundles.append("bundles[" + i + "] \"" + applicationBundles.get(i) + "\"\n");
+        }
+        dirConfigSource.writeConfig("application-bundles", String.format("bundles[%s]\n%s", applicationBundles.size(), bundles));
+    }
+
     protected void writeBootstrapConfigs(ComponentEntry... componentEntries) {
         dirConfigSource.writeConfig("platform-bundles", "");
         dirConfigSource.writeConfig("application-bundles", "");
@@ -126,4 +117,24 @@ public class ContainerTestBase {
         }
     }
 
+
+    public static class TestDeconstructor implements ComponentDeconstructor {
+
+        final TestOsgi osgi;
+
+        public TestDeconstructor(TestOsgi osgi) {
+            this.osgi = osgi;
+        }
+
+        @Override
+        public void deconstruct(long generation, List<Object> components, Collection<Bundle> bundles) {
+            components.forEach(component -> {
+                if (component instanceof ContainerTest.DestructableComponent vespaComponent) {
+                    vespaComponent.deconstruct();
+                }
+            });
+            bundles.forEach(osgi::removeBundle);
+        }
+    }
+
 }