Rework plugin config system #2

[DerEchtePilz]

Small follow-up for #596 because I was too quick

[willkroboth]

Just one bug on Velocity and some general comments about the tests.

Neither of these are big deals, but I noticed two ways that ConfigGenerationTests.java has different general formatting to other test files. First the test class and test methods here have public visibility, while the other classes have no visibility keyword. That's just a JUnit convention 🤷; if they don't need to be public, they get package visibility. SonarCloud complains about this :P

Second, tests like CommandExecutionTests or CommandNamespaceTests that use utility methods put those utilities above the tests. Here that would be all the validate... methods. I think utility methods on top makes sense? Like I'm reading top to bottom, I see the methods defined, then I see how they are used. The first time I read through ConfigGenerationTests I was a bit confused about what methods the tests were referencing. Not a big deal though because once I got to the bottom I knew what I was looking at and understood on my second read.

To make the coverage reports work, I had to add these dependencies to commandapi-codecov:

<dependency>
	<groupId>dev.jorel</groupId>
	<artifactId>commandapi-bukkit-plugin-common</artifactId>
	<version>${project.version}</version>
</dependency>
<dependency>
	<groupId>dev.jorel</groupId>
	<artifactId>commandapi-plugin</artifactId>
	<version>${project.version}</version>
</dependency>

From the coverage, tests seem pretty good. There's just a few branches missed in BukkitConfigurationAdapter#tryCreateSection:

ConfigGenerator#generate never returned null:

BukkitConfigurationAdapter#saveDefaultConfig was never run at all, and the default DefaultBukkitConfig was never created:

I think YamlConfiguration#saveToString could be helpful for testing those last two things. Maybe could check that saving the default config to file results in the correct String? Instead of including the full expected String in the test file directly, the expected config String could be loaded from another file.

[DerEchtePilz]

First the test class and test methods here have public visibility, while the other classes have no visibility keyword. That's just a JUnit convention 🤷; if they don't need to be public, they get package visibility.

Ah, okay, wasn't aware of that. Will change.

To make the coverage reports work, I had to add these dependencies to commandapi-codecov

Yeah, I am aware of that and it exists as a local change already.

ConfigGenerator#generate never returned null

A test for that exists locally already.

[willkroboth]

Still some parts of the code untested: the branches in BukkitConfigurationAdapter#tryCreateSection, the saveDefaultConfig method, and the rest is stuff saveDefaultConfig calls. Those methods look good but the edge cases are important for the functionality, so I think it would be nice to get them covered in tests.
Also, while BukkitConfigruationAdapter#complete is called, its effect of placing \n in the right places isn't verified, as the string version of a config file is never checked. Wouldn't have to be a big test config, just at least two options to make sure that a newline gets placed between the options. Maybe a test could also verify the indentation of comments on nested sections and options inside those sections.

[DerEchtePilz]

Still some parts of the code untested: the branches in BukkitConfigurationAdapter#tryCreateSection, the saveDefaultConfig method, and the rest is stuff saveDefaultConfig calls. Those methods look good but the edge cases are important for the functionality, so I think it would be nice to get them covered in tests.

I generally agree, but the tryCreateSection method is not too important now since we have one section and I honestly don't see that changing in the near future plus the features that are supposed to work, do work which is verified in manual tests.
saveDefaultConfig in my opinion also doesn't need tests since that is functionality that is easily verified in manual tests which did happen. Same with the newlines between options, easily verifiable in manual tests.

Before merging, I am also definitely going to do those manual tests a last time so that shouldn't be problem.

[willkroboth]

doesn't need tests since that is functionality that is easily verified in manual tests

🤷 Manual tests are fine, but it sounds tedious to me to redo them each time the relevant code changes. If someone else were to change this code, they might not know that the manual tests need to happen, or they might not handle all cases. That seems like the point of automated tests to me.

Granted, a lot of the code in the CommandAPI is uncovered since it was written before there was a testing framework. But, I like it when new changes are covered as to not make extra work to go back to later.