Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Add ExprAbsorbedBlocks test

Open F1r3w477 opened this issue 5 months ago • 11 comments

Problem

The Skript project has low test coverage. The ExprAbsorbedBlocks expression, which relies on the SpongeAbsorbEvent, had no automated tests.

Solution

This PR introduces robust tests for ExprAbsorbedBlocks.

  • Tests are broken down into simple, single-block absorption scenarios for each cardinal direction.

  • Each test runs in an isolated location to prevent contamination during consecutive test runs.

  • A cleanup and teardown process is used in each test block to avoid contamination.

Testing Completed

skriptTest

Supporting Information

This test was particularly difficult to stabilize due to limitations in the test runner's parser and race conditions in the server's block update queue. The final script structure represents a robust pattern for handling asynchronous, physics-based events.


Completes: none Related: #6158

F1r3w477 avatar Jul 20 '25 07:07 F1r3w477

Thanks for contributing to Skript's test coverage :) While this test is probably okay as–is, a timing-based approach is not the right solution to ensure cases don't collide.

IIRC blocks are metadata holders, so you could use a metadata tag to link a sponge block to a specific test case and check it in the event — this way the test also can't fail if an unrelated sponge happens to absorb blocks (though that is at best unlikely)

There's no need to check cardinal directions individually, just one test case that checks that the list of absorbed blocks is correct should be enough.

bluelhf avatar Jul 20 '25 11:07 bluelhf

Thanks for the feedback and the suggestion to use metadata. It's definitely a more elegant approach in theory.

After extensive debugging, the current implementation (using physically isolated tests with timing) was the only way I could create a test that is 100% stable and correctly fails the build when an assertion is broken.

I agree it's not the most concise solution, but it provides full test coverage where none existed before. As the contributing guidelines mention, "Tests don't have to be good code - often, testing edge cases pretty much requires weird code." This seems to be one of those cases where a more direct method was needed to work around the test environment's quirks.

I'm happy to continue exploring other options if you have a specific syntax in mind that avoids the parsing errors, but for now, this version is fully functional and reliable.

F1r3w477 avatar Jul 20 '25 14:07 F1r3w477

This test is unable to run without waiting. If we want there to be no waits, somewhere we will need to document that this is untestable.

F1r3w477 avatar Jul 20 '25 15:07 F1r3w477

This test is unable to run without waiting. If we want there to be no waits, somewhere we will need to document that this is untestable.

Well, in my posted screenshot, you can see the event is still being fired from the other tests as you have no delay between the start through cleaning up and setting block to water and block to sponge. Your first test has a wait 1 second before clean up, which ends up making the rest of the code for that test not execute.

Granted, it seems that the event will be fired without having any delays. I'm not sure if using an event in a non-junit testing environment is something that should be done. Would like to see what other team members think though.

Absolutionism avatar Jul 20 '25 15:07 Absolutionism

Granted, it seems that the event will be fired without having any delays. I'm not sure if using an event in a non-junit testing environment is something that should be done. Would like to see what other team members think though.

Sounds good, absorbed blocks is only accessible from within the event, so if others agree, then this is not testable in this context.

F1r3w477 avatar Jul 20 '25 15:07 F1r3w477

Why exactly are delays necessary here? I would expect the absorb event to be fired when the block is placed. Also, try to avoid changing blocks far from spawn, since it requires loading/generating new chunks and slows tests down.

sovdeeth avatar Jul 21 '25 02:07 sovdeeth

The assertion assert size of absorbed blocks is 1 with "Sponge should have absorbed 1 block" will intermittently read absorbed blocks as 2 if I remove waits.

My assumption is the test processes faster than the server processes the water physics changes. The test failure tends to be on consecutive test runs when using quickTest.

I can move the block setting closer to the test-location, I'll hold off on adjusting the test until we work through the concerns about waiting and using the event in the test.

F1r3w477 avatar Jul 21 '25 13:07 F1r3w477

The test works fine from one side if we don't wait. I updated the test to test from one side as @APickledWalrus suggested, and removed the waits.

If we need to test absorption from more than one side, I was unable to get it to work without making the test wait. As it sits now, there are no waits, we only absorb from one side, and the test takes place at test-location.

F1r3w477 avatar Jul 24 '25 18:07 F1r3w477

I've made the requested changes. I also added an assertion in the test block too.

I'm still not 100% sure this can be truly considered testable. If you do consecutive quickTest runs, every other run this test will fail.

When I run skriptTest it fails for Java21 every time.

Not sure what the issue is.

F1r3w477 avatar Jul 31 '25 16:07 F1r3w477

unfortunately tests where events have to be called are usually made using JUnit as you can specify the delay in ticks before the tests shut down. an example:

java: https://github.com/SkriptLang/Skript/blob/master/src/test/java/org/skriptlang/skript/test/tests/syntaxes/events/EvtVillagerCareerChangeTest.java skript: https://github.com/SkriptLang/Skript/blob/master/src/test/skript/junit/EvtVillagerCareerChangeTest.sk

converting to this would probably fix the inconsistency in the test's success

Efnilite avatar Jul 31 '25 17:07 Efnilite

I just find it strange that it runs fine in everything but Java 21. I'll consider figuring out turning it into a JUnit test, though I've never done JUnit tests before.

F1r3w477 avatar Jul 31 '25 19:07 F1r3w477