Add tests #14

vihaanthora · 2022-07-12T09:40:40Z

This PR adds two tests that test the HyperLocalPluginManager.java class. The resources/git-plugin/plugins folder contains the jpis of git-plugin. PipelineStepExtractor and QuasiDescriptor are added to facilitate testing. They are the same as the ones in pipeline-steps-doc-generator, except PipelineStepExtractor has been reduced to the functions required for testing. In HyperLocalPluginManager.java, the access modifier has been changed to public in L531 (as the method is required by pipeline-steps-doc-generator) and the unnecessary error stack trace removed in L539.

All other changes are formatting-related, to make the indentation and spacing uniform.

Once this PR gets merged, the artifact will be ready to use in pipeline-steps-doc-generator.

cc: @kwhetstone @arpoch @jmMeessen

kwhetstone

Vihaan and I talked about the test cases in our meeting. Looking forward to releasing the first version of the incrementals!

kwhetstone · 2022-07-12T15:26:57Z

src/main/java/hudson/MockExtensionLists.java

@@ -19,7 +19,8 @@
 import jenkins.model.Jenkins;

 /**
- * A mocked way to get at {@link ExtensionList}s. In {@code hudson} package due to protected access in {@link ExtensionList}.
+ * A mocked way to get at {@link ExtensionList}s. In {@code hudson} package due


Love that you added some comments to the class. It's always a good thing to do in order to help people understand what's happening in pieces of code.

kwhetstone · 2022-07-13T03:05:47Z

src/test/java/org/jenkinsci/pipeline_steps_doc_generator/PipelineStepExtractor.java

+            jenkinsHolderField.set(null, mockJenkinsHolder);
+
+            InitStrategy initStrategy = new InitStrategy();
+            executeReactor(initStrategy, pluginManager.diagramPlugins(initStrategy));


You are all good right up until here, then at this point you'll want to return the pluginManager. Then, we'll use the pluginManager in the test itself.

kwhetstone · 2022-07-13T03:31:22Z

src/test/java/org/jenkinsci/infra/tools/HyperLocalPluginManagerTest.java

+    public static void init() {
+        steps = pse.findSteps(pluginDir);
+        for (String plugin : steps.keySet()) {
+            LOG.info("processing " + plugin);


We don't want to keep the logs in the tests.

kwhetstone · 2022-07-13T04:26:23Z

src/test/java/org/jenkinsci/infra/tools/HyperLocalPluginManagerTest.java

+
+    @Test
+    public void areTotalStepsFortySeven() {
+        assertEquals(47, count);


This is a good place to return the pluginManger and query the plugin manager. You can specifically use the pluginManager methods to look for the git plugin step.

kwhetstone · 2022-07-13T04:35:30Z

src/test/java/org/jenkinsci/pipeline_steps_doc_generator/PipelineStepExtractor.java

+/**
+ * Process and find all the Pipeline steps definied in Jenkins plugins.
+ */
+public class PipelineStepExtractor {


I do also want to point out that having a separate class for setting up the HyperLocalPluginManager and running the reactor. This is a great idea!

vihaanthora · 2022-07-13T08:19:10Z

@kwhetstone I added a commit now, I have tried to address everything that we discussed yesterday. The only thing that is perhaps missing now is adding tests for diagramPlugins. In case we don't require it now, I will mark this PR as ready for review.

vihaanthora · 2022-07-13T08:22:57Z

pom.xml

@@ -43,6 +43,11 @@
      <artifactId>mockito-core</artifactId>
      <version>4.6.1</version>
    </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-inline</artifactId>


I got rid of the mockito error messages by adding an extra dependency as mentioned in the GitHub discussion here.

kwhetstone

This is great test refactoring! It's great to have the test running to make sure you've covered the green path for PluginManager.

kwhetstone · 2022-07-18T01:52:48Z

src/main/java/org/jenkinsci/infra/tools/HyperLocalPluginManager.java

@@ -305,7 +321,8 @@ protected Enumeration<URL> findResources(String name) throws IOException {
            List<URL> resources = new ArrayList<>();
            if (FAST_LOOKUP) {
                for (PluginWrapper p : activePlugins) {
-                    resources.addAll(Collections.list(ClassLoaderReflectionToolkit._findResources(p.classLoader, name)));
+                    resources


I'm not the largest fan of this lint change since it seems less clear to me.

kwhetstone · 2022-07-18T01:53:46Z

src/main/java/org/jenkinsci/infra/tools/HyperLocalPluginManager.java

@@ -400,21 +415,23 @@ public <T> List<T> findComponents(Class<T> type) {
    }

    /**
-     * This is pretty much a copy of the final ExtensionFinder.Sezpoz class from 1.651 fitted
+     * This is pretty much a copy of the final ExtensionFinder.Sezpoz class from
+     * 1.651 fitted


Note: something we might need to do in this repo is do a refresh on Sezpoz. A good ticket for this repository.

Sure, would like to know more about this in the office hours.

kwhetstone · 2022-07-18T01:54:43Z

src/main/java/org/jenkinsci/infra/tools/HyperLocalPluginManager.java

@@ -515,20 +528,20 @@ public void scout(ClassLoader cl) {
     * @param d descriptor
     * @return name of the plugin this class belongs to, or "core" if not found
     */
-    protected String getPluginNameForDescriptor(Descriptor<?> d) {
+    public String getPluginNameForDescriptor(Descriptor<?> d) {


I like this public as well especially since this is now a public utility.

kwhetstone · 2022-07-18T01:55:03Z

src/test/java/org/jenkinsci/infra/tools/HyperLocalPluginManagerInit.java

+        } catch (Exception ex) {
+            LOG.log(Level.SEVERE, "Plugin Manager failed to initialize", ex);
+        }
+        return pluginManager;


kwhetstone · 2022-07-18T01:55:22Z

src/test/java/org/jenkinsci/infra/tools/HyperLocalPluginManagerInit.java

+     *           If non-null, this can be consulted for ignoring some tasks. Only
+     *           used during the initialization of Jenkins.
+     */
+    private void executeReactor(final InitStrategy is, TaskBuilder... builders)


Right, this is pulled directly from the internals of Jenkins.

kwhetstone · 2022-07-18T01:56:14Z

src/test/java/org/jenkinsci/infra/tools/HyperLocalPluginManagerTest.java

+    }
+
+    @Test
+    public void TotalStepsShouldBeFortySeven() {


Cool. You can also make a comment for this method showing it's testing that the pluginStrategy loads itself correctly.

vihaanthora · 2022-07-19T14:01:06Z

I have made the required changes and added some comments.

Add tests

a803361

jmMeessen added the enhancement label Jul 12, 2022

jmMeessen requested review from jmMeessen and kwhetstone July 12, 2022 09:49

vihaanthora marked this pull request as draft July 12, 2022 16:11

kwhetstone suggested changes Jul 13, 2022

View reviewed changes

Refine test class

c358e15

vihaanthora commented Jul 13, 2022

View reviewed changes

vihaanthora force-pushed the tests branch from c358e15 to e79d73e Compare July 13, 2022 08:24

vihaanthora requested a review from kwhetstone July 13, 2022 18:00

kwhetstone approved these changes Jul 18, 2022

View reviewed changes

Add comments in the test class

4379ca3

vihaanthora force-pushed the tests branch from e79d73e to 4379ca3 Compare July 19, 2022 13:58

vihaanthora marked this pull request as ready for review July 19, 2022 13:58

jmMeessen merged commit 4891407 into jenkins-infra:master Jul 20, 2022

vihaanthora deleted the tests branch July 25, 2022 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests #14

Add tests #14

vihaanthora commented Jul 12, 2022 •

edited

kwhetstone left a comment

kwhetstone Jul 12, 2022

kwhetstone Jul 13, 2022

kwhetstone Jul 13, 2022

kwhetstone Jul 13, 2022

kwhetstone Jul 13, 2022

vihaanthora commented Jul 13, 2022 •

edited

vihaanthora Jul 13, 2022

kwhetstone left a comment

kwhetstone Jul 18, 2022

kwhetstone Jul 18, 2022

vihaanthora Jul 19, 2022

kwhetstone Jul 18, 2022

kwhetstone Jul 18, 2022

kwhetstone Jul 18, 2022

kwhetstone Jul 18, 2022

vihaanthora commented Jul 19, 2022

Add tests #14

Add tests #14

Conversation

vihaanthora commented Jul 12, 2022 • edited

kwhetstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vihaanthora commented Jul 13, 2022 • edited

Choose a reason for hiding this comment

kwhetstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vihaanthora commented Jul 19, 2022

vihaanthora commented Jul 12, 2022 •

edited

vihaanthora commented Jul 13, 2022 •

edited