Issues with Skript's testing system

[TPGamesNL]

There are currently multiple issues with Skript's testing system, so this thread hopefully summarizes (most of) it.

• ~~There are consistent errors in 1.9.4 and 1.12.2 from the test 3305-named inventory-item.sk (error).~~
  • ~~This causes ~75% of the tests (on those environments) to not be run and thus completely ignored.~~
  • EDIT: 1.9-1.12 no longer supported
• Parse errors are hard to find back, since the error telling you that the parsing failed doesn't mention where it failed.
• No tests have to run for the overal result to be a success.
  • This could happen when the server fails to start, see also https://github.com/SkriptLang/Skript/issues/4676
• If all tests are empty (you can try by giving an empty list to ScriptLoader#L678), the test is successful.
  • This scenario is unlikely, but I experienced it myself, after I messed something up in VariableString, since it's used in every test trigger.
• ~~There are overall not enough tests for the fundamental parts of Skript (e.g. https://github.com/SkriptLang/Skript/pull/3629#issuecomment-748050322 could probably have been prevented with a test).~~

One thing that I think could be useful for the testing system, is to add a system that generates a syntax tree from a few test scripts, and compares those syntax trees with versions of that syntax tree previously generated for the scripts. I already have a project that can do the syntax tree generation, I'd just need to edit it and make this system work with it. The benefit of such a system, is that it's easy to add an existing script (without addons), by letting the system generate a syntax tree, and add it to the testing system.

[quick007]

also sometimes if you change smth in a function and reload the skript with the function, it will think the function has been deleted. (reloading again fixes the issue)

[TPGamesNL]

also sometimes if you change smth in a function and reload the skript with the function, it will think the function has been deleted. (reloading again fixes the issue)

You should open a new issue for that

[TPGamesNL]

• There are consistent errors in 1.9.4 and 1.12.2 from the test 3305-named inventory-item.sk (error).
  • This causes ~75% of the tests to not be ran and thus completely ignored.

~~When removing the assert that causes these errors, the following tests fail consistently:~~

Failed:
  item comparisons (on 2 environments)
    {_b} is a skeleton skull (it shouldn't be) (on paper-1.12.2)
    {_b} is a skeleton skull (it shouldn't be) (on paper-1.9.4)
  potion effects (on 1 environments)
    the item should have had the speed potion type (on paper-1.9.4)

EDIT: 1.9-1.12 no longer supported

[TPGamesNL]

~~Additionally, some of the tests fail at random (link): ~~

~~The failed build: ~~ Lime fixed at #4962

[TPGamesNL]

~~IMO no tests should be ran during Gradle building either~~ done in #4091

[TPGamesNL]

~~The AssertionErrors not being caught, is caused by~~ https://github.com/SkriptLang/Skript/blob/d33f96ba9f854a9abf969387c009961ce7f8d913/src/main/java/ch/njol/skript/lang/TriggerItem.java#L97 ~~not catching AssertionError (because it's not an Exception, it's an Error)~~

EDIT: fixed in https://github.com/SkriptLang/Skript/pull/4767

[TheDGOfficial]

Errors are not supposed to be catched anyway, general practice is to catch Exception, but if you really want to catch all errors, including weird ones that probably JVM should handle itself, i.e. OutOfMemoryError, then you want Throwable, it is the root of all exceptions that can be thrown by throw statement.

[TPGamesNL]

Errors are not supposed to be catched anyway, general practice is to catch Exception, but if you really want to catch all errors, including weird ones that probably JVM should handle itself, i.e. OutOfMemoryError, then you want Throwable, it is the root of all exceptions that can be thrown by throw statement.

We could catch Error (or just AssertionError specifically), set the errored flag and re-throw it

[TheLimeGlass]

~~I get this error when attempting to use gradlew skriptTestDev~~

* What went wrong:
Execution failed for task ':skriptTestDev'.
> Could not set unknown property 'standardInput' for task ':skriptTestDev' of type org.gradle.api.DefaultTask.

Fixed in https://github.com/SkriptLang/Skript/pull/4845

[bluelhf]

I get this error when attempting to use gradlew skriptTestDev

* What went wrong:
Execution failed for task ':skriptTestDev'.
> Could not set unknown property 'standardInput' for task ':skriptTestDev' of type org.gradle.api.DefaultTask.

This can be solved by calling createTask with a second argument, JavaExec, and omitting the doFirst and javaexec from the task definition

[TPGamesNL]

~~After #4441, the unit test configs should be updated with each Paper release. Preferably we wouldn't have to do this.~~ Fixed by https://github.com/SkriptLang/Skript/pull/4513

[TPGamesNL]

More random fails:

[TPGamesNL]

The testing system does not take delays in tests into account, so when using a delay (e.g. wait 1 tick) in a test, there's no guarantee the code after it will run, and therefore it often does not run.

[TPGamesNL]

~~Another recent issue causing tests to fail occasionally: when tests are done, the server crashes because it thinks it is no longer responding: https://pastebin.com/FeiYGkRc~~

~~EDIT: fixed in https://github.com/SkriptLang/Skript/pull/4900~~

~~EDIT 2: I guess not yet~~

EDIT 3: should be actually fixed now for real https://github.com/SkriptLang/Skript/pull/4906

[TPGamesNL]

(suggested at https://github.com/SkriptLang/Skript/pull/4845#discussion_r927586147) We could split the different version tests up into multiple workflows, so they are ran concurrently.

GH does impose the limit of 20 concurrent jobs (for the whole org), so having one workflow per supported MC version might not be the greatest idea.

One idea could be the following distribution:

• newest MC version on newest Java version
• oldest MC version on oldest Java version
• the rest of the MC versions, not ran concurrently