Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit Tests #149

Open
CryptoMorin opened this issue Dec 6, 2021 · 3 comments
Open

Unit Tests #149

CryptoMorin opened this issue Dec 6, 2021 · 3 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed

Comments

@CryptoMorin
Copy link
Owner

CryptoMorin commented Dec 6, 2021

There are currently some issues with the unit tests that must be solved, most of them has to do with Maven. Also, some test related classes are left hanging around intentionally just in case they are needed in the future.

Right now there is a master profile tester which handles tests in general, and subsidiary profiles for running the unit tests on different Minecraft versions (named after the minor version 18, 17, 16, etc.)
So you can just run these tests with a simple Maven command: mvn clean package -Ptester,18

The first problem is that running these tests for lower versions like 1.16 won't work because surefire will use 1.16 jar to compile and test.
This will obviously fail because the XSeries library is designed to be compiled on the latest version and handle older versions during runtime.
I need to find a way to compile the tests using the latest version of Spigot, but test/run them using a lower version. I tried playing around with options like <dependenciesToScan> without luck.

Another insignificant issue is that I can't find a way to run all these profiles one after another automatically to test the library for all the versions.

And the last abomination is this. Without this delay the server won't completely load:

Thread.sleep(2000L);

@CryptoMorin CryptoMorin added documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed labels Dec 6, 2021
@portlek
Copy link
Contributor

portlek commented Dec 6, 2021

The first problem is that running these tests for lower versions like 1.16 won't work because surefire will use 1.16 jar to compile and test.

i guess separating modules in terms of version could be help for this situation.

@rbluer
Copy link

rbluer commented Dec 7, 2021

Here is an idea about why the thread.join() is not working as expected.

Since thread.start() is not blocking and does not wait for the VM to actually spawn and start a new thread, it could be that when the vm initially hits the thread.join() it is not recognizing that thread is running yet, and therefore it does not block to await for the termination of the thread's run function. In the api docs, it does not explain what the behavior is if join is attempted on a non-running thread; if it just passes through, then it could explain the behavior that you are seeing.

Perhaps even a slightest delay after the start could allow join to finally block?

    thread.start();
    System.out.println("post-start");
    Thread.sleep( 50L );
    System.out.println("pre-join");
    thread.join();
    System.out.println("post-join");

Such that "Done!" should appear in the logs between "pre-join" and "post-join".

Might be able to get by with a value of less than 50 ms, such as 5 ms or even less, or it may need to be more. Personally, I'd probably bump that up to a higher value to accommodate slow/old hardware/vms when using it in a real unit test environment. I just wanted to illustrate with the 50 ms that its probably only needing a very short sleep time to give the vm a chance to create and get the thread started.

Not sure it this is exactly what is going on, but I just thought I'd throw my idea in to the ring. ;)
Blue

PS... My project has been using XSeries for well over a year now, but it is not included in any of your dependencies due to limitations of the use of gradle, of which is not supported in git's dependency reporting. Anyway, this is an excellent resource that has allowed me to scrap tons of old internal code "trying" to deal with block compatibility. Since the project is over 7+ years old, it became a nightmare to support newer blocks, especially with the flattening. So XSeries has really allowed me to focus on the plugin, and not the blocks, which is greatly appreciated! Thanks! :)

@CryptoMorin
Copy link
Owner Author

CryptoMorin commented Dec 12, 2024

I figured out the Thread.sleep() issue. It was simply that Bukkit's server instance ran on a new separate thread instead of reusing the startup thread. I used a traditional polling while loop to wait until Bukkit.getServer() is available. We can't really do anything other than that.

After many years, it seems like the solution to the main issue was not simple at all...
This solution is far from perfect. In fact, I've marked all the issues with a Warning tag.
Link to changes

So basically when you do mvn compile test -Ptester,8, you're running your compile goal with the tester and 8 profiles as well. There doesn't seem to be a way to run two goals with two different set of active profiles in the same command. You'd have to run it as separate commands:

mvn compile
mvn test -Ptester,8

With that being said, we wanted to compile our project (both main sources and test sources) with the latest Java version (but with Java 8 as target) and the latest Minecraft version, but run the actual tests themselves (from surefire plugin), using a different Java version and a different Minecraft version (different dependency)

We could simply just run the goals for the first step since the requirements are already configured as defaults:

mvn compiler:compile compiler:testCompile

We could just run compiler:compile as compile I just wanted to show they're from the same plugin.

However, for step two, we need to configure a few things:

Using a different Minecraft version is pretty straightforward, we just need to use a profile:

<profile>
    <id>8</id>
    <properties>
        <spigotVersion>1.8.8-R0.1-SNAPSHOT</spigotVersion>
    </properties>
</profile>

This will replace the ${spigotVersion} Maven property which is used by the Maven dependency:

<dependency>
    <groupId>org.spigotmc</groupId>
    <artifactId>spigot</artifactId>
    <version>${spigotVersion}</version>
    <scope>provided</scope>
</dependency>

And it's assigned to the latest Spigot version by default (without using any profiles)

Secondly, we need to run the tests using a separate Java version. Configuring the Java version for the maven compiler plugin is pretty easy with the <source> and <target> inside their <configuration>, because it uses the main JAVA_HOME and simply tells the Java compiler to target lower versions.
However, we can't tell Java's runtime to run a jar using a lower Java version, so we need to give the Maven surefire plugin an entirely different Java path to use using the <jvm> configuration:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-surefire-plugin</artifactId>
    <version>...</version>
    <configuration>
        <includes>
            <include>**/*.java</include>
        </includes>
        <jvm>${testJavaPath}</jvm>
    </configuration>
</plugin>

We use another Maven property here so we can change it using profiles again! For example, MC v1.8.8 can't run with Java 21, so we need to give it Java 8. A typical Java 8 path on Windows would be:

<profile>
    <id>8</id>
    <properties>
        <spigotVersion>1.8.8-R0.1-SNAPSHOT</spigotVersion>
        <testJavaPath>C:\Program Files\Java\jre-1.8\bin\java</testJavaPath>
    </properties>
</profile>

We simply don't define this property if a higher version, like v1.21 is used which supports the latest Java version.

Warning

We could use toolchains to properly build this across different platforms, however setting up the Maven toolchain is quite
time consuming. Considering that we need multiple Java versions, and users will have to install those versions if they want
to test older versions. I don't think there's any solution to at least omit the full Java path into something more "universal"

Now we're ready to run the second step:

mvn test -Ptester,8

Warning

There's a small issue with this solution. This solution exploits the fact that compile and compiler:testCompile goals will not run again since they're in a UP-TO-DATE state after being run once.
Rarely, the sources might change after running mvn compile compiler:testCompile, even if you don't actually change anything in the classes. This results in the test goal to run compile and compiler:testCompile goals again, but this time with our -Ptester,8 profiles. And this will break everything for the reasons explained above.
More about this issue at the end.

Now, like I said, you can't really run multiple commands using Maven. You can only run multiple goals with a single set of active profiles. So it'll be really annoying to wait for one command to finish and run the second one. We can use exec-maven-plugin to execute multiple commands as separate goals:

<plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>exec-maven-plugin</artifactId>
    <version>x</version>
    <configuration>
        <executable>mvn</executable>
    </configuration>

    <executions>
        <execution>
            <id>compile</id>
            <phase>none</phase>
            <goals>
                <goal>exec</goal>
            </goals>
            <configuration>
                <arguments>
                    <argument>compiler:compile</argument>
                    <argument>compiler:testCompile</argument>
                </arguments>
            </configuration>
        </execution>
        <execution>
            <id>test</id>
            <phase>none</phase>
            <goals>
                <goal>exec</goal>
            </goals>
            <configuration>
                <arguments>
                    <argument>test</argument>
                    <argument>-Ptester,8</argument>
                </arguments>
            </configuration>
        </execution>
    </executions>
</plugin>

Now we can run two goals:

mvn exec:exec@compile exec:exec@test

however, every time we want to test with a different server version, we have to modify our pom.xml, so we could use a Maven property for the test goal instead

<argument>test</argument>
<argument>-Ptester,${testVer}</argument>

now to run a test we use:

mvn exec:exec@compile exec:exec@test -DtestVer=8

Warning

Since most people use the Maven version bundled with their IntelliJ, Maven executable path is not actually added/exported as environmental variable which causes the excec plugin to not find the mvn program. You'd have to configure the full path of Maven IntelliJ plugin in that case:

<plugin>
      <groupId>org.codehaus.mojo</groupId>
      <artifactId>exec-maven-plugin</artifactId>
      <version>x</version>
      <configuration>
          <executable>IntelliJInstallationPath\plugins\maven\lib\maven3\bin\mvn.cmd</executable>
      </configuration>
</plugin>

This is just extra work and yet another reason that a cross-platform solution is needed.

Warning

Another issue with maven exec plugin is that, it won't pass other arguments to either goals. For example, if you wanted to
use -X for a verbose output, it'll only apply to the exec goal, not the goals that it runs (i.e. compile, compiler:testCompile and test)

Warning

Another insignificant inconvenience is the fact that the goals run by exec plugin aren't colored.

Note

This whole exec plugin solution would've not practically worked unless we disabled incremental compilation.
When the second step (exec:exec@test -DtestVer=8) is run, the dependency is also changed, this causes
the test goal to recompile both the main source and test sources.
(-X Debug message: [INFO] Recompiling the module because of changed dependency.)

This can be fixed by simply adding the <useIncrementalCompilation>false</useIncrementalCompilation>
to the maven-compiler-plugin for the tester profile. Which keeps incremental compilation enabled for
our normal compilation and exec:exec@compile but disables it when exec:exec@test is executed.

However, it still seems like this affects incremental compilation for the first step? Might want to do more testing to confirm.

Warning

And finally... This issue is kinda weird and it just happened all of the sudden during the process of developing this solution...
For some reason, MC v1.8.8 is able to run using the JDK21 toolchain configured for the surefire plugin. The versions can be confirmed from the startup logs. More specifically, it always runs with the Java version defined by JAVA_HOME variable. It even says the correct JDK path in the logs Toolchain in maven-surefire-plugin: JDK[...] I have no idea why this happens - it works - fuck it - I'll leave it alone for now. This seems to be still an issue when I tested v1.16 and v1.18 with the message: Unsupported Java detected (65.0). Only up to Java 18 is supported. It looks like using the jvm option of surefire plugin solves this issue, but that option requires a home path directly. No combination of the surefire and toolchain plugin configs seem to work.

Obviously all of these issues could be more easily solved with Gradle with a cleaner gradle task<ver>, but we're going to keep using Maven for now...
As for running all these at once, we can do that too, but I realized it's kinda unnecessary most of the times. If anyone's willing to make a configuration for that, I'll happily use it.
Any improvements to this solution is welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request opinions Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants