Make the output of compileJava an input of neow3jCompile to have a consistent caching behavior

[Lorenzobattistela]

#1016

So @gsmachado I've followed your instructions, and I think this is the right way. I'm sure i dont understand 100% how it all happens, but Im getting along. Now I need to test this changes (specially the syntax around Spec), but I'm not quite sure how, running ./gradlew tests or ./gradlew integrationTests is failing, is there any setup needed? If so, where can I find it?

[gsmachado]

That's a good start! 💪

So... the tests didn't run because, most likely, the code has a compilation problem. Did you try ./gradlew clean compileJava?

Also, tests are missing. It's rule that for each new feature, tests should be developed. Unit tests and integration tests. For the case of gradle-plugin module, we just develop integration tests. See here.

One way is to create a test case where the cacheable=false also running the compilation twice. If the task outcome of the second execution is FROM_CACHE, then the test didn't pass. If the outcome is SUCCESS, then it passed. We can discuss this in Discord, if you want. 👍

[gsmachado]

Thinking out loud: we might think about using an @InputDirectory variable and setting the output of the compileJava task from the Java Plugin. This way we would rely on the (awesome) cache of Java Plugin, out of the box.

[Lorenzobattistela]

Thinking out loud: we might think about using an @InputDirectory variable and setting the output of the compileJava task from the Java Plugin. This way we would rely on the (awesome) cache of Java Plugin, out of the box.

From the docs: This will cause the task to be considered out-of-date when the directory location or contents have changed.
So you mean basically we set the output dir as the input dir for caching, then if it has changed, the task would be considered out of date?

[Lorenzobattistela]

@gsmachado At first, I've completed the initial approach so we can continue from here. I created the test case, but the second runBuild call is outputing UP_TO_DATE. I assume this happens because the files are exactly the same, so for the test I guess we have to change something for it to recompile.

[gsmachado]

@gsmachado At first, I've completed the initial approach so we can continue from here. I created the test case, but the second runBuild call is outputing UP_TO_DATE. I assume this happens because the files are exactly the same, so for the test I guess we have to change something for it to recompile.

That's a good progress!

So... let me elaborate a bit more on why I came up with this comment:

In the past, I was playing with neow3j and realized that, sometimes, when I changed a smart contract (i.e., a .java file / smart contract using the devpack, etc), the neow3j compiler was not being invoked -- at all. That's why I created #1016. I thought that setting a flag (e.g., cacheable) and configuring the neow3jCompile task to not be cached by the Gradle daemon would be enough. It seems it's not, as you highlighted in your test -- the UP-TO-DATE outcome always appears.

Thinking a bit better, I remembered about past experiments with Gradle, as well as the logic of inputs and outputs. Also, by reading the code, I realized that I went with a simpler implementation and did not properly use the output of compileJava being funneled into the neow3jCompile task.

Then, I suggest you try the following:

1. Add the following method (replaceMethodName()) to the TestCaseUtils class:

    protected static void replaceMethodName(String filePath, String oldMethodName, String newMethodName) throws IOException {
        Path path = Paths.get(filePath);

        // Read the file content into a String
        String content = new String(Files.readAllBytes(path), StandardCharsets.UTF_8);

        // Replace the method name
        content = content.replaceAll("\\b" + oldMethodName + "\\b", newMethodName);

        // Write the modified content back to the file
        Files.write(path, content.getBytes(StandardCharsets.UTF_8));
    }

This method intends to replace a string on a file. The intention here is to change the method name of the testing smart contract. We could try to change anything else (like, the class name), but since the smart contract used for testing is pretty simple, I'd just go with changing the method name.

2. Add a replaceMethodName() method to the GradleProjectTestCase class:

    protected GradleProjectTestCase replaceMethodName(String oldMethodName, String newMethodName)
            throws IOException {
        TestCaseUtils.replaceMethodName(this.smartContractSourceFile.getAbsolutePath(), oldMethodName, newMethodName);
        return this;
    }

3. In this line of your test, use the replaceMethodName() as, for example, .replaceMethodName("test", "test-modified"). As you might imagine, this intends to change the source and then run the compilation again.

4. Then, if you run the test, you will realize the following:

• The first buildResult will return SUCCESS.
• The second buildResult will return UP-TO-DATE.
• BUT, if you check the second buildResult object, more specifically checking the outcome of ALL tasks, the following will return:
  

What does this mean?

It means that, in the second buildResult, the compileJava task was not cached. This is the intended behavior since we changed the Java smart contract source (the input of the compileJava task), and then Gradle automatically realized that it has to recompile the smart contract to .class again. However, the neow3jCompile task simply ignored the fact that it has to recompile the smart contract. This is a bug. It's not an intended outcome.

Then, my initial goal to add a cacheable flag is maybe something we can ditch by now. We actually don't need it. I tried to fix something (that I didn't know was a bug) by introducing a feature. Not good. 😄

So, how do we solve it? How do we make this test pass? (i.e., the second buildResult should return SUCCESS to neow3jCompile task after we change the source of the smart contract)

Some suggestions for you:

• Replace the whole cacheable thing in the Neow3jCompileTask class to:

    @InputDirectory
    public abstract DirectoryProperty getBuildDirectory();

• Add the following to the end of the Neow3jCompileTask.class configuration:

            task.getProject().getPluginManager().apply(JavaLibraryPlugin.class);
            JavaCompile compileJava = (JavaCompile) project.getTasks().getByName(JavaPlugin.COMPILE_JAVA_TASK_NAME);
            task.getBuildDirectory().set(compileJava.getDestinationDir());
            task.dependsOn(JavaPlugin.COMPILE_JAVA_TASK_NAME);

• Besides making sure that the neow3jCompile should always be triggered if smart contracts are modified, we also need to make sure that if the smart contract source does NOT change then the neow3jCompile should NOT be triggered. I expect you to come up with this test as well. 😸

Happy coding.

[Lorenzobattistela]

@gsmachado this other approach seems to be working well, if I'm not mistaken. The only problem I'm seeing is that one test case is breaking, probably because of directory / path stuff. Look:
Neow3jCompilePluginIntegrationTest > testTaskCompilationError_className_notFound() FAILED
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>

here:
assertTrue(testCase.getBuildNeow3jOutputDir().exists());

It seems that the build output directory does not exist after a compilation error.
I think this is a consequence of this lines:

JavaCompile compileJava = (JavaCompile) project.getTasks().getByName(JavaPlugin.COMPILE_JAVA_TASK_NAME);
task.getBuildDirectory().set(compileJava.getDestinationDir());

Maybe they're hitting different destinations?

[gsmachado]

Good work.

I requested few (minor) changes in the test files.

In addition to that, I believe that the logic of these lines, which the test is failing now, can be changed -- because is not entirely correct.

I would propose to delete these lines and add this one:

assertFalse(testCase.getBuildNeow3jOutputDir().exists());

Meaning that: if the neow3jCompile fails (this line), then the build dir related to neow3j smart contracts should also not exist as a result.

[Lorenzobattistela]

Done @gsmachado . Thanks for all the help. Now I understand much better how neow3j works, and I'm sure this will help on future contributions.

[gsmachado]

Looks good. 👍 ✅

[csmuller]

@Lorenzobattistela I made a small change in the main branch. Please merge the neow3j main branch into your fork here. We're trying to make the unit tests run in fork pull-requests. Thx!

[gsmachado]

@Lorenzobattistela congrats on the first PR merged to neow3j. 🎉