Miscellaneous code cleanup #399

basil · 2023-01-13T17:19:40Z

Fixes IDE warnings and makes the codebase use Java 11 conventions where applicable. While I was here I fixed a minor bug in a call to ProcessBuilder, fueled mostly by my recent usage of the same API in a different pull request in this repository (with which the existing code is now consistent). This passed testing in jenkinsci/bom.

basil · 2023-01-13T17:25:05Z

pom.xml

+        <exclusion>
+          <groupId>org.apache.ant</groupId>
+          <artifactId>ant</artifactId>
+        </exclusion>


No longer needed, as we removed the only usage of org.apache.tools.ant.filters.StringInputStream in this PR.

basil · 2023-01-13T17:25:10Z

pom.xml

+      <exclusions>
+        <exclusion>
+          <groupId>com.google.code.findbugs</groupId>
+          <artifactId>jsr305</artifactId>
+        </exclusion>
+      </exclusions>


No longer needed, as we switched to SpotBugs annotations in this PR to be consistent with the rest of the Jenkins ecosystem.

basil · 2023-01-13T17:25:18Z

src/main/java/org/jenkins/tools/test/CliOptions.java

@@ -232,7 +232,7 @@ public String getMavenPropertiesFile() {
        return mavenPropertiesFile;
    }

-    @Nonnull
+    @NonNull


Switching to SpotBugs annotations to be consistent with the rest of the Jenkins ecosystem.

basil · 2023-01-13T17:25:33Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

@@ -191,7 +191,7 @@ public PluginCompatReport testPlugins()

        // Scan bundled plugins
        // If there is any bundled plugin, only these plugins will be taken under the consideration for the PCT run
-        UpdateSite.Data data = null;
+        UpdateSite.Data data;


Removing an unnecessary initialization due to the subsequent if/else block.

basil · 2023-01-13T17:25:39Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

@@ -459,7 +459,7 @@ private TestExecutionResult testPluginAgainst(MavenCoordinates coreCoordinates,
        System.out.println(String.format("%n%n%n%n%n"));

        File pluginCheckoutDir = new File(config.workDirectory.getAbsolutePath() + File.separator + plugin.name + File.separator);
-        String parentFolder = StringUtils.EMPTY;
+        String parentFolder = "";


Reducing our dependency on Apache Commons Lang. The fewer third-party dependencies, the more maintainable the code.

basil · 2023-01-13T17:51:24Z

src/test/java/org/jenkins/tools/test/VersionComparatorTest.java

@@ -27,9 +27,9 @@

 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertThat;
+import static org.hamcrest.MatcherAssert.assertThat;


Migrating from a deprecated method to its non-deprecated equivalent.

basil · 2023-01-13T17:51:46Z

src/test/java/org/jenkins/tools/test/VersionComparatorTest.java

@@ -40,7 +40,7 @@
 */
 public class VersionComparatorTest {

-    private static final ImmutableMap<String, Integer> OPERAND_CONVERSION = ImmutableMap.of(
+    private static final Map<String, Integer> OPERAND_CONVERSION = Map.of(


Reducing our dependency on Guava. The fewer third-party dependencies, the more maintainable the code.

basil · 2023-01-13T17:52:28Z

src/test/java/org/jenkins/tools/test/model/MavenPomTest.java

+                normalizeXmlString(Files.readString(Paths.get(getClass().getResource(resource).toURI()), StandardCharsets.UTF_8)),
+                normalizeXmlString(FileUtils.readFileToString(pom, StandardCharsets.UTF_8))


Reducing our dependency on Apache Commons I/O. The fewer third-party dependencies, the more maintainable the code.

While this was previously using the default character set, this seemed like a bug because our pom.xml files in Git repositories should always be encoded in UTF-8 (as their XML prologs often declare).

basil · 2023-01-13T17:59:02Z

src/test/java/org/jenkins/tools/test/PluginCompatTesterTest.java

@@ -282,15 +279,15 @@ public void testWithoutAlternativeUrl() throws Throwable {
    }

    @Ignore("TODO broken by GH protocol changes. Requesting user data access via https on local execution")
-    @Test(expected = Test.None.class)
+    @Test


Removing an unnecessary parameter: Test.None.class is the default value.

basil · 2023-01-13T17:59:15Z

src/test/java/org/jenkins/tools/test/hook/JacocoHookTest.java

@@ -33,36 +33,36 @@ public void testCheckMethod() {
    }

    @Test
-    public void testAction() throws Exception {
+    public void testAction() {


Removing an unnecessary throws clause.

jtnord

looks good except the potential for a deadlock in the process launching to get the java version

jtnord · 2023-01-17T17:07:47Z

src/main/java/org/jenkins/tools/test/PluginCompatTester.java

@@ -938,19 +939,19 @@ public static String getMavenModule(String plugin, File pluginPath, MavenRunner
        if (absolutePath.endsWith(plugin)) {
            return plugin;
        }
-        String module = absolutePath.substring(absolutePath.lastIndexOf(File.separatorChar) + 1, absolutePath.length());
+        String module = absolutePath.substring(absolutePath.lastIndexOf(File.separatorChar) + 1);


🤷 what on earth was it warning about?

src/main/java/org/jenkins/tools/test/model/PluginCompatTesterConfig.java

jtnord · 2023-01-17T17:22:49Z

src/main/java/org/jenkins/tools/test/model/PluginCompatTesterConfig.java

+        try {
+            process.waitFor();
+        } catch (InterruptedException e) {
+            throw new IOException("interrupted while getting Java version", e);
+        }


we were not waiting for the process to terminate before trying to read its output.

for good reason see above ;-)

readAllBytes will block until the end of stream is reached - (ie the process has terminated, or otherwise closed its output stream) thus we can not have partial data.

the argument splitting fix is still valid.

jtnord · 2023-01-17T17:27:34Z

src/main/java/org/jenkins/tools/test/model/PluginCompatTesterConfig.java

+        try {
+            process2.waitFor();
+        } catch (InterruptedException e) {
+            throw new IOException("interrupted while getting full Java version", e);
+        }


see similar comment about this and introducing a blocked state.

jtnord · 2023-01-17T17:32:28Z

src/main/java/org/jenkins/tools/test/model/PluginRemoting.java

@@ -166,7 +169,7 @@ public PomData retrievePomData() throws PluginSourcesUnavailableException {
            packaging = StringUtils.trimToNull((String)packagingXPath.evaluate(doc, XPathConstants.STRING));

            String parentNode = xpath.evaluate("/project/parent", doc);
-            if (StringUtils.isNotBlank(parentNode)) {
+            if (parentNode != null && !parentNode.isBlank()) {


this is a common enough idiom here that I wonder if in in hindsight if you had considered introducing a helper class in this module. The two not conditions is not the best for legibility

basil · 2023-01-17T18:50:05Z

All feedback is addressed from my perspective - let me know if there are any other changes you would like

jtnord · 2023-01-18T13:41:01Z

downstream closed source build is passing - this is good to merge from my perspective.

basil added the work-in-progress label Jan 13, 2023

basil force-pushed the cleanup branch 2 times, most recently from 47c8f04 to 105b886 Compare January 13, 2023 17:35

basil commented Jan 13, 2023

View reviewed changes

basil force-pushed the cleanup branch from 105b886 to 50ab5c3 Compare January 14, 2023 00:15

Miscellaneous code cleanup

23c8d90

basil force-pushed the cleanup branch from 50ab5c3 to 23c8d90 Compare January 14, 2023 17:04

basil added a commit to basil/bom that referenced this pull request Jan 14, 2023

Test jenkinsci/plugin-compat-tester#399

396334b

basil added internal and removed work-in-progress labels Jan 15, 2023

basil marked this pull request as ready for review January 15, 2023 03:51

basil requested a review from a team as a code owner January 15, 2023 03:51

basil requested a review from jtnord January 15, 2023 03:52

jtnord requested changes Jan 17, 2023

View reviewed changes

basil added 2 commits January 17, 2023 10:41

Add a gobbler

488ced3

SpotBugs

e775e95

jtnord self-requested a review January 18, 2023 13:37

jtnord approved these changes Jan 18, 2023

View reviewed changes

basil merged commit 2e67c6f into master Jan 18, 2023

basil deleted the cleanup branch January 18, 2023 14:40

basil mentioned this pull request Jan 18, 2023

Upgrade PCT to latest jenkinsci/bom#1683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous code cleanup #399

Miscellaneous code cleanup #399

basil commented Jan 13, 2023 •

edited

Loading

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

jtnord left a comment

jtnord Jan 17, 2023

jtnord Jan 17, 2023

jtnord Jan 17, 2023

jtnord Jan 17, 2023

basil commented Jan 17, 2023

jtnord commented Jan 18, 2023

		normalizeXmlString(Files.readString(Paths.get(getClass().getResource(resource).toURI()), StandardCharsets.UTF_8)),
		normalizeXmlString(FileUtils.readFileToString(pom, StandardCharsets.UTF_8))

Miscellaneous code cleanup #399

Miscellaneous code cleanup #399

Conversation

basil commented Jan 13, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord 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

basil commented Jan 17, 2023

jtnord commented Jan 18, 2023

basil commented Jan 13, 2023 •

edited

Loading