Skip to content

Conversation

TheLimeGlass
Copy link
Contributor

@TheLimeGlass TheLimeGlass commented Aug 1, 2025

Problem

The current event system of Skript will error if any single event cannot be cancelled in it's registered listening events.
For example, if an event was registered that listened to a cancellable event and a non cancellable event. Skript would either pass it or fail it depending on the registration order. Not ideal.

See existing TODO note placed by Njol:

return true; // TODO warning if some event(s) cannot be cancelled even though some can (needs a way to be suppressed)

Solution

This pull request fixes that issue, and produces a suppressible warning if any of the events cannot be cancellable. If all events cannot be cancelled, then a full error is produced like current behavior.

JUnit beneficial changes needed to accommodate:

  • Allowed all test registration to be present in the DEV ENVIRONMENT for self testing.
  • DEV ENVIRONMENT now uses the JUnit test jar to allow for usage of JUnit and custom test syntaxes.
  • JUnit objectives can now be completed programmatically with static method EffObjectives#complete and not just through a Skript ran effect.

Made usage of the built in JUnit custom registration system I made awhile ago but never used.
We now have a valid example of making custom events for tests.

Testing Completed


Completes: none
Related: none

@TheLimeGlass TheLimeGlass requested review from a team as code owners August 1, 2025 10:03
@TheLimeGlass TheLimeGlass requested review from UnderscoreTud and erenkarakal and removed request for a team August 1, 2025 10:03
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Aug 1, 2025
@sovdeeth
Copy link
Member

sovdeeth commented Aug 1, 2025

I've asked you repeatedly to follow the correct PR template. You appear to be deliberately choosing to not use it in favor of the outdated one we've replaced. It's not the biggest issue but it shows an unacceptable disregard for our contribution standards and the team.
Please update the PR description to use the current template.

@TheLimeGlass
Copy link
Contributor Author

I've asked you repeatedly to follow the correct PR template. You appear to be deliberately choosing to not use it in favor of the outdated one we've replaced. It's not the biggest issue but it shows an unacceptable disregard for our contribution standards and the team. Please update the PR description to use the current template.

Updated

@sovdeeth
Copy link
Member

sovdeeth commented Aug 1, 2025

Updated

Thank you. The fields at the end are incorrect (should show Completed: and Related:). You are also missing a section for any testing you completed. Please use the correct template from the get-go next time :)

@TheLimeGlass
Copy link
Contributor Author

Updated

Thank you. The fields at the end are incorrect (should show Completed: and Related:). You are also missing a section for any testing you completed. Please use the correct template from the get-go next time :)

Updated

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Aug 1, 2025
@skriptlang-automation skriptlang-automation bot added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. and removed needs reviews A PR that needs additional reviews labels Aug 5, 2025
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having to execute the events, I'd recommend using StructParse which should simplify the test implementation:
https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/test/runner/StructParse.java
https://github.com/SkriptLang/Skript/blob/master/src/test/skript/tests/syntaxes/structures/StructParse.sk (note that this tests a nested example)

@skriptlang-automation skriptlang-automation bot removed the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Aug 7, 2025
@TheLimeGlass
Copy link
Contributor Author

TheLimeGlass commented Aug 11, 2025

Rather than having to execute the events, I'd recommend using StructParse which should simplify the test implementation: https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/test/runner/StructParse.java https://github.com/SkriptLang/Skript/blob/master/src/test/skript/tests/syntaxes/structures/StructParse.sk (note that this tests a nested example)

It does use the parse sections. Not the structure, because the point is to test cancellation events, not a wrapper using another parsed structure. It should remain using the same implementation a Skripter would interact with. Avoid the probability of the other structure causing logging desync or not true parsing.

Example being how async effects and expressions within sections from skript reflect don't properly print logger errors currently due to the thread being async or multi threading parsing option.

@APickledWalrus
Copy link
Member

Rather than having to execute the events, I'd recommend using StructParse which should simplify the test implementation: https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/test/runner/StructParse.java https://github.com/SkriptLang/Skript/blob/master/src/test/skript/tests/syntaxes/structures/StructParse.sk (note that this tests a nested example)

It does use the parse sections. Not the structure, because the point is to test cancellation events, not a wrapper using another parsed structure. It should remain using the same implementation a Skripter would interact with. Avoid the probability of the other structure causing logging desync or not true parsing.

Example being how async effects and expressions within sections from skript reflect don't properly print logger errors currently due to the thread being async or multi threading parsing option.

The execution of the events doesn't need to be tested/occur though. Something like this would would test the same thing without having to handle executing the events:

parse:
	results: {cancellable::1::*}
	code:
		on cancel events test 1:
			cancel event

test "multiple cancellable events":
	assert {cancellable::1::*} is "An on cancel events test 1 event can be triggered for multiple events, some of which cannot be cancelled." with "Failed to parse or grab cancel events 1 warning"

And then it doesn't have to be a JUnit test anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants