Handle multiple output source directories

[KevinMcT]

• Modification to handle changes made in gradle 4.0
  • See https://docs.gradle.org/4.0/release-notes.html
    Location of classes in the build directory

[KevinMcT]

filtering prevents a NoSuchFileException from being throw down in fb/sb internals as it will attempt to access directories even though they don't exist.
A better approach would be to handle this exception where it arises and skip this "potentially" costly operation here.

[KengoTODA]

Almost LGTM, but could you reproduce existing problem by unit test? This change has no test code.

[KevinMcT]

Yeah, I'll look into it when I get back from holidays. :) 13. mar. 2018 02:07 skrev "Kengo TODA" <notifications@github.com>:

…

***@***.**** commented on this pull request. Almost LGTM, but could you reproduce existing problem by unit test? This change has no test code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACQQlhtoqw_hGSJUn-cd9O7n49GmhH5mks5tdynKgaJpZM4SU4jU> .

[terribleherbst]

Any progress on this?

[KevinMcT]

Forgot a bit about it, on paternity leave atm. Picking this up now. Will update with test coverage latest thursday.

[KevinMcT]

@terribleherbst @KengoTODA having some trouble writing tests to replicate the failure scenario / success scenario with the fix.

The tests I've written pass succesfully regardles of fix or not, however when I copy the project created by the tests and run the tasks manually through console/IDE, I get the expected results.

Simple example of the test I wrote which should "fail". But spotbugsMain task reports Success.

`public class IssueTest {
@rule
public TemporaryFolder folder= new TemporaryFolder();

@Before
public void createProject() throws IOException {
    String buildScript = "plugins {\n" +
            "  id 'java'\n" +
            "  id 'groovy'\n" +
            "  id 'com.github.spotbugs' version '1.6.1'\n" +
            "}\n" +
            "version = 1.0\n" +
            "repositories {\n" +
            "  mavenCentral()\n" +
            "  mavenLocal()\n" +
            "}\n" +
            "dependencies {\n" +
            "    compile 'org.codehaus.groovy:groovy-all:2.4.14'\n" +
            "}\n";
    File buildFile = folder.newFile("build.gradle");
    Files.write(buildFile.toPath(), buildScript.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE);

    File sourceDir = folder.newFolder("src", "main", "groovy");
    File to = new File(sourceDir, "Bar.groovy");
    File from = new File("src/test/java/com/github/spotbugs/Bar.groovy");
    Files.copy(from.toPath(), to.toPath(), StandardCopyOption.COPY_ATTRIBUTES);
}

@Test
public void testShouldFail() throws Exception {
    BuildResult result = GradleRunner.create().withProjectDir(folder.getRoot())
            .withArguments(Arrays.asList("spotbugsMain")).withPluginClasspath().build();
    assertThat(result.task(":compileGroovy").getOutcome(), is(TaskOutcome.SUCCESS));
    assertThat(result.task(":classes").getOutcome(), is(TaskOutcome.SUCCESS));
    assertThat(result.task(":spotbugsMain").getOutcome(), is(TaskOutcome.NO_SOURCE));
}

}`

Got some idea's ? :)

[terribleherbst]

I guess the test case should be created in a different way, at least I have a different scenario.

I do have the following structure:

src/main/java/Bar.java
src/main/groovy (empty)
src/test/java/ (empty)
src/test/groovy/BarTest.groovy

Could you try this?

[gcwiak]

Any updates?

[KevinMcT]

Yeah, back @ work now. Will see if I can sort this out over the next couple of days.

[fholger]

We're running into the same issue in one of our projects where we have modules with mixed Scala/Java code. This is currently a blocker for our migration from FindBugs to SpotBugs.

I already verified that this pull request fixes the issue for us, so very interested in seeing this merged. If I can offer any assistance, let me know :)

[KevinMcT]

@fholger your more than welcome to pitch in here. Writing these test's are doing my head inn.

I've been messing about with the gradle testkit, modeling the new test's after the existing tests to no avail.
Test is basically the same as the one mentioned earlier, creating a project which relies on the previous release of plugin: 1.6.1 which should result in failure.

Unit test report:
:compileJava NO-SOURCE
:compileGroovy
:processResources NO-SOURCE
:classes
:spotbugsMain

BUILD SUCCESSFUL in 8s
2 actionable tasks: 2 executed

Copying the project created from the unit test and running task via IDE/cmd:
:compileJava NO-SOURCE
:compileGroovy UP-TO-DATE
:processResources NO-SOURCE
:classes UP-TO-DATE
:spotbugsMain NO-SOURCE

BUILD SUCCESSFUL in 0s
1 actionable task: 1 up-to-date

Taken from a run in one of our projects:
:gradle-plugin:compileJava NO-SOURCE
:gradle-plugin:compileGroovy
:gradle-plugin:processResources UP-TO-DATE
:gradle-plugin:classes
:gradle-plugin:spotbugsMain FAILED

FAILURE: Build failed with an exception.

• What went wrong:
  Execution failed for task ':gradle-plugin:spotbugsMain'.

No classes configured for SpotBugs analysis.

BUILD FAILED in 5s
3 actionable tasks: 2 executed, 1 up-to-date

So in theory, the unit test SHOULD reply with NO-SOURCE for spotbugsMain, but for some reason it responds with SUCCESS.. Why I havn't been able to figure out, currently my only suspicion is that there is something funky going on with the test kit runnner, cause common sense argues that if it behaves correctly through cmd and IDE execution the only remaining sinner is the test kit runner.

@fholger if you have any good idea's I'm all ears.

I'm thinking that a solution to this could be to establish a secondary stand alone project within the project running integration tests.

So we'll create a sub project which consists of java/scala/groovy/kothlin classes and make the sub project depend on the plugin. This project would then act as the main verification that the plugin acts as advertised. We're doing something similar for our own custom plugin, Thoughts @KengoTODA

PS: @terribleherbst that is one of the test cases which should be covered once we get this testing working. But fyi this pull request does handle that.

[fholger]

@KevinMcT I quickly hacked together a "minimal" sample of the setup we have in our project and put it into a test. It works as expected in that it fails on current master, but works with your fix. Although "minimal" is a bit of a misnomer, since it depends on Scala and thus needs to download Scala packages. Maybe you can replace Scala with something more lightweight, but either way, this is the test I ended up with:

package com.github.spotbugs;

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.util.Arrays;
import org.gradle.testkit.runner.BuildResult;
import org.gradle.testkit.runner.GradleRunner;
import org.gradle.testkit.runner.TaskOutcome;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;

public class CombinedSourcesTest {
    @Rule
    public TemporaryFolder folder= new TemporaryFolder();

    @Before
    public void createProject() throws IOException {
      String buildScript = "plugins {\n"
        + "    id 'java'\n"
        + "    id 'scala'\n"
        + "    id 'com.github.spotbugs' version '1.6.1'\n"
        + "}\n"
        + "version = 1.0\n"
        + "repositories {\n"
        + "    mavenCentral()\n"
        + "    mavenLocal()\n"
        + "}\n"

        + "apply plugin: 'java'\n"
        + "apply plugin: 'scala'\n"

        + "sourceSets.main.scala.srcDirs = ['src/main/java', 'src/main/scala']\n"
        + "sourceSets.main.java.srcDirs = []\n"

        + "ext.scalaFullVersion = '2.10.3'\n"
        + "dependencies {\n"
            + "compile 'org.scala-lang:scala-library:2.10.7'\n"
        + "}\n";
      File buildFile = folder.newFile("build.gradle");
      Files.write(buildFile.toPath(), buildScript.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE);

      File javaSourceDir = folder.newFolder("src", "main", "java");
      File scalaSourceDir = folder.newFolder("src", "main", "scala");
      File to = new File(javaSourceDir, "Foo.java");
      File from = new File("src/test/java/com/github/spotbugs/Foo.java");
      Files.copy(from.toPath(), to.toPath(), StandardCopyOption.COPY_ATTRIBUTES);
    }

    @Test
    public void testCombinedScalaJavaSources() {
        BuildResult result = GradleRunner.create().withProjectDir(folder.getRoot())
                .withArguments(Arrays.asList("spotbugsMain")).withPluginClasspath().build();
        assertThat(result.task(":spotbugsMain").getOutcome(), is(TaskOutcome.SUCCESS));
    }
}

[KevinMcT]

@fholger I found the issue. Much to the credit of your example.

The gradleRunner seems to ignore the fact that we're specifying a version for the plugin in the buildscript. As such is only uses the available local code. (Which sort of makes sense.) But on the other hand it ruins my initial take on the test.

So after conferring with one of my colleagues I think we've found a fair enough test proposal.

I'll push a second PR with only the test in it, which will fail. And update this PR with the test as well which will run the test successfully.

Given that proof, @KengoTODA I reckon you've got validation to submit this PR.

The 2nd failing PR can then be abandoned as there is no point point merging that in first producing an erroneous build just to fix it with a 2nd commit.

[KevinMcT]

Bah fcked up a bit on the conflict resolution >.< Serves me right for getting so used to gerrit..

[KengoTODA]

Sorry but we still have conflict, because we recently merged #3 and #18.

[KevinMcT]

@KengoTODA #28 is running without the proposed fix.

Hope this is good enough.

PS. Don't merge #28 💀 :)

[KengoTODA]

Fix seems good. Please explain your change in CHANGELOG.md, then we can merge this change.

[KevinMcT]

@KengoTODA flow got a bit interrupted as I didn't use a feature branch for this PR. But requested changes have been added and conflicts resolved.

Good to merge? 😺

[KengoTODA]

LGTM