-
-
Notifications
You must be signed in to change notification settings - Fork 404
add expr length tests #8052
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
base: dev/feature
Are you sure you want to change the base?
add expr length tests #8052
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation needs to be in tabs, not spaces.
Maybe some tests using substring and joining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always nice to have more tests!
I think substring and joining should be tested elsewhere. ie: In |
assert length of " " is 3 with "Length of a string with only whitespace should be correct" | ||
assert length of "😂" is 2 with "Length of a Unicode emoji should be calculated correctly" | ||
|
||
test "type conversion for length": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test is necessary - it's not handled by ExprLength but rather internally (and should be tested elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the test for the undefined variable. I'll remove that assertion.
Regarding the length of "12345" test, my intention was to test the expression with a string literal that contains numeric characters. Since ExprLength is registered for %strings%, this directly tests its intended functionality.
To avoid future confusion, I'll renamed the test block from "type conversion for length" to "numeric string length"
assert {_lengths::2} is 2 with "Second element of looped lengths should be correct" | ||
assert {_lengths::3} is 6 with "Third element of looped lengths should be correct" | ||
|
||
test "string interpolation": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also not really necessary and should be handled in a different test focused on VariableString behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this test would ideally live in a dedicated VariableString.sk
file.
Given that overall test coverage is a known issue Skript is working to improve, would it be acceptable to keep this integration test here for now? This ensures this case is covered now, rather than waiting until a VariableString.sk
file is created.
It can always be moved to a more appropriate file later on.
Problem
The Skript project currently has very low test coverage for its existing syntax, making it difficult to ensure changes don't introduce regressions. Specifically, the core length of expression was untested.
Solution
This PR adds a new syntax test file,
ExprLength.sk
, to provide unit test coverage for the length of expressions.The tests cover behavior for:
String literals and variables.
Edge cases like empty strings, whitespace, and Unicode characters.
Automatic type conversion from numbers.
The list-looping behavior when applied to a list of strings.
String interpolation.
Testing Completed
Added the
ExprLength.sk
test. The full test suite was run via .\gradlew skriptTest and passed successfully.Supporting Information
None
Completes: none
Related: #6158