Automated tests using PackTest

[misode]

Revisiting automated tests for our modules. See earlier prototype in #777.

Tests are written in *.mcfunction files and run using the PackTest mod. See the readme there for an explanation on the syntax in test files.

This PR will be considered ready when:

• The tests can be run in development
• The tests can be run in CI using packtest_runner
• PackTest has a release version compatible with the tests written in this PR (v1.3)
• Tests for a variety of modules have been written
  • Not all modules need to have tests in this PR, some can be done later
  • Others may never get them if writing automated tests would be infeasible
• Contributing instructions has been updated to explain tests

[misode]

This PR is ready for review, so have a go at trying to run the tests on your machine!

I'm going to keep writing tests for the remaining modules but we can merge this PR before every modules has tests.

[SpecialBuilder32]

Are there any concerns with this running after the commit / release step? I know we need the packs after beet is done with them to test, but it would be nice if the packtest prevented release if there were failures.

[misode]

That's a fair comment and I considered doing that. We could delay the release to gm4.co because that happens in a separate action step (Commit release and Push release), but since Modrinth and Smithed release happens during beet build that it would be a bit tricky. We could separate the publishing logic out of beet which might not be a bad idea.

For now I kept it last because I was a bit concerned with how long these tests could take and wouldn't want to needlessly delay the release. It looks like the tests only take 40s right now so that is probably fine.

Tests will run in pull requests anyways so we should be able to catch most issues early. And when a test fails on master we can investigate anyways. Still an improvement over not knowing that a bug was created and hearing about it months later in an issue on GitHub.

[SpecialBuilder32]

Alright I took a peek at this and I think it looks pretty good! Tests look pretty easy to create and that's hopefully super powerful going forward. Theres a few points I want to just point out or briefly discuss.

1. Keeping test files in the released pack
   I feel a little strange about releasing packs with the test files still intact, though theres alot of facets to that.
   Pros:

• No test files in the release means adding tests won't trigger an update patch, which is probably preferable.
  Cons:
• Since half of the test files (structures, predicates ect) are mixed in with regular datapack resources, we wouldn't be able to simply remove the tests directory.
• Would complicate the build process, since we'd need to first emit a pack with tests to hand to packtest, and then reemit the pack without tests for release.

2. Pack load errors
   One of the things I was most excited about the first iteration of packtest was the ability to see game output log errors in the GH action. This kinda shows up, but it doesn't fail action. I think this leads more to my next point though

3. Action Annotations
   This is something I looked into some earlier but got too confused by, but maybe you know some tricks I couldnt find in google. I would love if the failed tests (and pack load errors) showed up as action annotations (to my best understanding, just specially formatted messages to stdout). Right now they're just reported as "action exited with exit code 1". This might be a whole other PR for improving our action logging though.

[misode]

@SpecialBuilder32

1. I don't think it is a huge problem that tests are included in the released data packs. Currently apart from the tests directory only structures have been added, and they all start with test_, so automatic filtering is possible. An advantage of including the tests is that we are absolutely certain that we're releasing and testing the exact same files.
   As for triggering a patch bump, after this early phase of adding tests to all modules, most tests will only change when re release for new versions or when that module is getting updated anyways.
   If we decide we don't want the tests files in the release, we could be to split the github action in two jobs that run in parallel, one building the release and the other running the tests.

2. I agree, this is something I want to add to packtest in the future. However correctly detecting whether something is an actual error is not trivial. I need to look into this, possible add some mixins, especially to correctly identify from which data pack the error came.

3. Yes I had planned this but completely forgot. Looking at the GitHub documentation, it looks fairly straightforward. I'm going to try to add this in the next release.

I have added both 2 and 3 to the packtest roadmap

[SpecialBuilder32]

I kinda like the idea of splitting our current action into build/test, at least so it's a little more cleanly organized in the action summary. Is that possible?

[Bloo-dev]

I tested this in-game in a The Void preset superflat world and it worked as expected. I can not confirm the lack beacons described by @SpecialBuilder32 in a related conversation on discord.
I've also had the chance to look at the code now and I must say I really like the syntax of the tests; they integrate seemlessly into mcfunction, and I'm certain everyone can write these without to much brain-jello as a side effect.

I have a couple of questions/notes still:

• how will this affect our GitHub action compute time? On the free plan we have limited compute time as far as I know, will we be able to run these tests without exhausting out compute time?
• the "Push Release" step of our pipeline will likely be subject to change soon (see issues Build/Release Pipeline: Modules released on Smithed and Modrinth are labled with very generous version coverage #940 and Show a range of supported versions instead of just 1.X gm4.co#38), so I am infavor of @SpecialBuilder32's suggestion of splitting our actions into more discrete chunks, i.e. at least build & test for now. Should that be done in this PR? Depends on how much work that is.

[misode]

@Bloo-dev

how will this affect our GitHub action compute time? On the free plan we have limited compute time as far as I know, will we be able to run these tests without exhausting out compute time?

It shouldn't. Public repos have unlimited action minutes, even for organizations. Looking at the action time, it seems to add roughly 40 seconds to every build.

I am infavor of @SpecialBuilder32's suggestion of splitting our actions into more discrete chunks, i.e. at least build & test for now. Should that be done in this PR? Depends on how much work that is.

I have split the workflow into two jobs. Still todo is stripping the tests from the build job, and do less work in the test job (like not building libraries separately, all the readme stuff, etc)

[SpecialBuilder32]

This looks like it runs the release build twice, once for the build and once for test. Is it possible to reuse the files in test or was this done to run the jobs in parallel. I don't think theres that much to strip out of the build that would benefit us.

I think I've also decided its fine to distribute test files? It might allow for some better debugging for advanced users (for example on modded servers) or point more people to the existence of packtest.

[misode]

This looks like it runs the release build twice, once for the build and once for test. Is it possible to reuse the files in test or was this done to run the jobs in parallel. I don't think theres that much to strip out of the build that would benefit us.

@SpecialBuilder32 If we wanted to reuse the same build then there was no point in splitting the action in two jobs. I was fine with the original workflow too 🤷

[SpecialBuilder32]

I wanted to split it so we could get status badges for build and test separately. Can different tasks not reuse the same files / work on different runners?

[misode]

Can different tasks not reuse the same files / work on different runners?

They can, using artifacts, but from experience on this repo it has a significant overhead (taking 10-20 seconds). I do see the advantage of having separate status badges so I'll keep it split.

I don't think building it twice is a big problem, it happens in parallel and we can cut away stuff we don't need for testing, so overall the action will be faster.

[misode]

@SpecialBuilder32 Pack load errors will now show up red in the log and will cause the action to fail. (see below)

I tried implementing the annotations, and found that it would be difficult to do for us because we have the beet build step. So when the server loads these data packs it can't know where those tests were defined in the source (without some heuristics and folder scanning, or an explicit source map from beet). I decided to leave that feature out for now.

[SpecialBuilder32]

I thought you could make an annotation without pointing it to a file, just having it essentially copy a message to the action summary. Pointing it to a source file was an optional thing for better debugging

[misode]

Edit after I saw Special's message: Oh I didn't even think to try that. I'll give it one more go.
Update: Success!

Original message:

I'm happy with this PR in its current state. Future work can include:

• Adding more tests of course
• Add github annotations for errors
• Send a notification in discord when the test fails on master
• If the tests turn out to be reliable, only release when the test succeeds

[misode]

All requested changed implemented and it has been 2 weeks, so merging...