Skip to content

Add expr active item test #8078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

F1r3w477
Copy link

Problem

The Skript project has low test coverage. The ExprActiveItem expression had no automated tests.

Solution

This PR adds tests for ExprActiveItem, covering its behavior with single and multiple entities, as well as syntax aliases.

Testing Completed

Added the ExprActiveItem.sk syntax test. The full project test suite was run via .\gradlew skriptTest and passed successfully.

Supporting Information

The positive case (an entity actively using an item) was intentionally not tested, as automating specific AI behavior can be unreliable.


Completes: none
Related: #6158

@F1r3w477 F1r3w477 requested a review from a team as a code owner July 23, 2025 22:05
@F1r3w477 F1r3w477 requested review from Burbulinis and TheMug06 and removed request for a team July 23, 2025 22:05
@F1r3w477 F1r3w477 changed the base branch from master to dev/feature July 23, 2025 22:06
@Burbulinis
Copy link
Contributor

What about tests for setting the entity's active item and confirming that they're correct?

@F1r3w477
Copy link
Author

What about tests for setting the entity's active item and confirming that they're correct?

From my PR description:

The positive case (an entity actively using an item) was intentionally not tested, as automating specific AI behavior can be unreliable.

@APickledWalrus APickledWalrus added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jul 24, 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.

Tests look good, but I don't think they need to be separate in separate "test" cases. One would suffice (I would call it "ExprActiveItem" or "Active Item")

@F1r3w477
Copy link
Author

I restructured to have one test that is "Active Item" for the single entities and another that is "Active Item on Multiple Entities" for multiple entities

@sovdeeth
Copy link
Member

I don't think this is worth adding. There's no test for the actual behavior the syntax provides, since as you said it's not easily testable.

@F1r3w477
Copy link
Author

The test was quick to make. It's here to add if you all want it. If not, that's fine. If everyone decides it shouldn't be added, should it be tracked somewhere that it's not testable?

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