Terasology: New API testing module

[Cervator]

While reviewing and merging #2306 I detected a tiny API violation of SemVer that was able to slip past detection earlier since we don't yet have any ability to actually test the module API on engine changes.

While we're in Alpha and flying half-blind like this it is probably very hard to avoid trivial violations like that which can be so subtle. Maybe we can be accepting of that on a case-by-case basis until Beta or when we no longer can say we are aware of and able to fix all live modules. Thoughts?

In either case we need a better way to detect API violations. We have the ApiScraper already to produce _what_ to test, how about creating a unit test module that'll exercise every single entry minimally, including a bit of logic to compare what's tested vs. what exists? Maybe there's some clever way to work code coverage into it, this is something we should hit 100% on.

Related: #2159, #1975, https://github.com/MovingBlocks/Terasology/wiki/Modding-API

[GabrielXia]

Heya :-)

I implement an API test for interface Context and class ComponenetFieldUri, check it out:
ApiTest. In short it compares the signatures of current public & protected methods (method name, modifiers, parameter, return type, exceptions) to the signatures registered. Is it what we want? Now the signature registering stuff is kind of manual. I think maybe it's better to gather some comments before going on testing the huge size of API ;-)

Thanks for your suggestions!

[Cervator]

Bit of a belated response here but yep, that seems about right :-)

At first I was thinking simply using each method in some test, since in that case any method signature change would cause a failure. But if there are validators like that it may be even more correct!

We still would want actual unit tests that exercise stuff too, eventually. I wonder if having the method signature validators would cause code coverage analysis to think that code is covered? That might be something to look into. IMHO simply validating the method signatures shouldn't count as covering the code itself.

This reminds me a bit about #2851 which was using a generator that makes unit tests. Really bare ones but it might have some potential for API testing I wonder? Pinging @iaronaraujo for reference.

[iaronaraujo]

I just made a pull request with a tutorial about how to use EvoSuite to create unit tests!
https://github.com/MovingBlocks/Terasology/pull/2906

[Cervator]

Thanks @iaronaraujo for the nice write-up! And for popping up on this issue as I wanted to run something by both you and @GabrielXia :-)

Today during my day job I got into thinking about Semantic Versioning, auto-detecting the correct scope to increment, and guaranteeing that a pending release has bumped the version number accordingly (optionally over-scoping - such as incrementing major even if there isn't a technical API violation, but for instance all save games will fail to load in the new version)

Between the auto-generated tests from @iaronaraujo and the method signature detection by @GabrielXia could we combine the two to arrive at a complete snapshot of the entire public API? Then between scanning/scraping what is marked as public API vs what we have covered by those tests and comparing vs. state in the master branch we could in theory get a perfect image of API state. Plus a good scaffold to then improve the generated API tests to also include additional more meaningful unit tests that functionally validate API methods.

Image the automatic build test of a new engine PR:

• If it introduces breaking changes by removing a parameter on a public method the API tests will fail. We know it'll need a major version increase.
  
  
  
  • Even if the author adjusts the tests we can run the tests as of the master branch against the new code and we know an incompatible change is being introduced
  
  
• If it introduces a new public method (or variant) then it requires a minor version increase (comparing the public API scan before and after would detect it)
• If it matches perfectly (so ignoring changes in method content or private/protected methods) then it remains a patch release

If any API change is thus detected the PR can be blocked until it includes a version bump appropriate for the change (which would need community approval). Comparing to master would show if said version bump has already happened, in which case the API change would be OK for SemVer (still subject to review) as we have already done the correct increment.

Going with that would guarantee that the version in develop is always correct, as it'll be bumped as part of a merged PR that required the bump. As such there's no actual need to worry about a bigger bump at build/release time (at present I vary minor vs patch manually, usually at release time, and had thought I'd need to set it as a Jenkins release action parameter)

We could eventually expand this to modules and other projects.

What do you think?

[GabrielXia]

Thanks @iaronaraujo for the cool tutorial and @Cervator for the interesting propose, I like it !!! 👍

So if I understand correct, we are going to do:

• For each API:
  
  
  
  • Auto-generated tests using EvoSuite for the code coverage
  
  
  • Method signature detection to API violation
  
  
  • More meaningful test
  
  
• Compare the API we have vs API we tested vs the state in master branch for documentation

However, I have a few questions as follow:

1. Do we ignore protected methods?
2. About auto-detecting the correct scope to increment: Since unit returns 2 results: pass or not pass, how can we distinguish 3 scope: major, minor and patch? Or maybe we just print something to show which scope to increment?

Is there something else I miss? I guess I have some misunderstandings, thanks for pointing it out :-) Also sorry about the delay

[iaronaraujo]

That's a pretty nice idea! Regarding EvoSuite, I think it will fit this scope very well! It will still be necessary to check the tests generated to see if they make sense and also make some code adjustments, but after these tasks are done, the tests would be very useful to check how much a new PR would change the engine.

I also had the same doubts as @GabrielXia . I think it's easy to manually check the scope of the change by looking at the tests' results and the code changes on Github, but how would we do that automatically?

[Cervator]

@GabrielXia that sounds about right yep

1. Yeah I think protected should be ignored since such methods wouldn't be part of the Modding API

2. That's where the PR vs master comes in handy:

• If you run the generated API tests from the master branch (likely stored somehow in Jenkins or Artifactory as of the latest release) against the source of a PR then if the author changed a method signature in a backwards incompatible way the tests will fail. We know there is an API violation requiring a major version bump
  
  
  
  • If the tests included with the PR themselves do pass and the version has been bumped to the next major version since the prior release (from the master branch) then the PR does validate as a viable major release element. We may or may not accept that (community discussion & decision)
  
  
• If you run master tests against the PR and they pass, but code coverage (maybe actual code coverage, or some custom scanning we do for the tests / API stuff) for public API methods isn't 100% then the PR is introducing new functionality (new methods / method variants). It indicates a minor release.
  
  
  
  • Again if tests included with the PR do pass + cover 100% + there is a minor version bump included then everything is prepared and we can merge if we choose to. Minor releases are routine.
  
  
• If the master tests pass and coverage is still 100% then it is a patch release and we're fine (patch gets incremented by default after a release).

The API tests should be stored in a second source set distinct from the regular one in engine-tests I believe (src/api-tests/java maybe?) - they count as something other than unit tests. By keeping it as a different set of tests I believe we can do stuff like not counting it as test coverage for "regular" tests and such, but I haven't experimented with that.

That may also allow us to later run the tests from an archive somehow, produced by the previous release.

[Cervator]

Making a quick site note here before I forget. There are two tricky places where API tests may need a little extra to detect full SemVer compliance:

• A class previously whitelisted via @API or another method (like being whitelisted at the package level) has the API flagging removed. This would not be caught by an API test within the engine simply testing the class methods - the failure would only occur at regular runtime with a usage from a module. Not hard to catch, but an easy case to miss.
• A service previously available within the game Context is no longer published there. This would (at first anyway) only affect engine-published services, like the WorldRenderer. Likely solution would be to simply have a big test that goes and retrieves every known service to make sure it is available, from a full environment. That would need a list of all "standard" services which would probably be pretty handy anyway for documentation, much like the ApiScraper. At least some can be found by looking for the @Share annotation but you can also register a service manually.

@GabrielXia & @iaronaraujo: I'm hoping we can finally get back to this now that the pressure from GSOC is over - might be a good option for a mini-GSOC type thing! Maybe the two of you would still be interested. @kaen and @oniatus have also made some neat progress on the automated testing angle as well.

[oniatus]

We have one nice attempt with a unit based check from @GabrielXia at #2919.
A different approach would be to generate something like a golden master file for our API.
This would be an extension of the ApiScraper:

1. Extend the ApiScraper to output a list of all packages, classes and (this is new) all methods with their signatures
2. Create one class with a main method to manually write this output to something like an API-File
3. Create an automated unit test which calls the api-scraper and then compares to output against the stored api-file.

This could give us something similar, maybe with a bit less programming effort for the cost of putting one textfile somewhere in the project resources.
Once we have another demo-PR for this approach we can decide which way to go or if we want to try out a different approach.

Edit: Created https://github.com/MovingBlocks/Terasology/issues/3239 for this task.