Refactor project structure and enhance testing framework #5
Conversation
|
@coderabbitai why did you skip review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2160 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1029
🛠 Debug Information Gathered
Additionally, your PR title is set to "@coderabbitai", which is configured as your Recommended fixes:
Would you like me to trigger a review of this PR now? |
@coderabbitai yes trigger a review. |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai just change title and body. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 49365 🏁 Script executed: Length of output: 298
Title: Body: ## Changes
### Project Structure
- Reorganized scripts into `scripts/libs/` and `scripts/utils/` directories
- Moved test files from `test/` to `tests/` directory
- Consolidated test framework into `scripts/utils/testframework.sk`
### CI/CD
- Added GitHub Actions workflow for automated testing (`test-skripts.yml`)
- Configured workflow to run on `stable` and `dev` branches
- Updated wiki publishing workflow to trigger on `stable` branch
### Testing Framework Enhancements
- Improved assertion syntax for better readability
- Expanded documentation with comprehensive feature descriptions
- Added experimental parse section and log inspection capabilities
- Enhanced test execution with autorun control and conditional execution
### Test Updates
- Updated all test files to use new assertion syntax
- Improved test isolation and error tracking
- Added test utilities for controlled test fixturesYou can update these directly in the GitHub UI by editing the PR. |
devdinc
changed the title
WalkthroughThis pull request migrates the test framework from a root-level implementation to a new structured approach under Changes
Sequence DiagramsequenceDiagram
participant Parser as Script Parser
participant Registry as Test Registry
participant Executor as Test Executor
participant Assertions as Assertion Engine
participant Output as Console Output
Parser->>Registry: skriptTest event fires<br/>(test "name" [when condition])
Registry->>Registry: Register test with<br/>condition evaluation
alt User Command: run tests
Executor->>Registry: Query all registered tests
Registry-->>Executor: Return test list
end
loop For each test
Executor->>Executor: Evaluate when condition
alt Condition met
Executor->>Assertions: Execute test block
Assertions->>Assertions: Collect assertions<br/>and errors
Assertions-->>Executor: Return results
end
end
Executor->>Output: Print summary<br/>(passed/failed counts)
Output-->>Executor: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
.github/workflows/test-skripts.yml (1)
37-56: Redundant directory creation.
mkdir -p build/libsis called three times across steps (lines 39, 44, 50). The-pflag makes subsequent calls harmless, but consolidating these into the first step improves readability.Suggested consolidation
- name: Pre-create plugin directory run: | mkdir -p build/libs mkdir -p build/libs/skript-reflect - name: Download Skript-Reflect run: | - mkdir -p build/libs curl -L -o build/libs/skript-reflect.jar \ https://github.com/SkriptLang/skript-reflect/releases/download/v2.6.1/skript-reflect-2.6.1.jar - name: Download Routines from JitPack run: | - mkdir -p build/libs curl -L --fail -o build/libs/skript-reflect/routines-core.jar \docs/testframework.md (1)
78-84: Add language specifiers to code blocks.Several code blocks are missing language identifiers. Based on static analysis, the blocks at lines 81, 96, 328, 336, and 351 should specify a language for proper rendering.
Suggested fixes
For the internal storage format examples (lines 96-99, 328-330), use
textorplaintext:-``` +```text -test.sk::tests::<script>::<test name>For the output format examples (lines 336-338, 351-353), also use `text`: ```diff -``` +```text <X>/<Y> tests passed```diff -``` +```text [Skript] [TEST FAILURE] <test name> <optional message>For the identifier format example (lines 130-133): ```diff -``` +```text <script>/<test name></details> Also applies to: 94-99, 326-331, 334-339, 349-354 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d11538f8b12788f841ccaa8f61148ce1f6edfe6a and 20704f10fd6e44c41d7ec2a26567332f9d00d44c. </details> <details> <summary>📒 Files selected for processing (17)</summary> * `.github/workflows/publish-wiki.yml` * `.github/workflows/test-skripts.yml` * `docs/testframework.md` * `scripts/libs/functionsv2.sk` * `scripts/libs/pdc.sk` * `scripts/libs/routines.sk` * `scripts/libs/singlelinesection.sk` * `scripts/utils/configreloadv2.sk` * `scripts/utils/testframework.sk` * `scripts/utils/worldproperties.sk` * `test/functionsv2.sk` * `test/main.sk` * `test/routines.sk` * `testframework.sk` * `tests/functionsv2.sk` * `tests/routines.sk` * `tests/singlinesection.sk` </details> <details> <summary>💤 Files with no reviewable changes (4)</summary> * test/routines.sk * test/functionsv2.sk * test/main.sk * testframework.sk </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/testframework.md</summary> 81-81: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 96-96: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 328-328: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 336-336: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 351-351: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (14)</summary><blockquote> <details> <summary>.github/workflows/publish-wiki.yml (1)</summary><blockquote> `4-4`: **Branch trigger alignment looks correct.** The change from `main` to `stable` aligns with the repository's branching strategy where `stable` serves as the primary branch for releases. </blockquote></details> <details> <summary>.github/workflows/test-skripts.yml (1)</summary><blockquote> `59-66`: **Script preparation logic is reasonable.** The rsync approach and selective file removal handle the test environment setup appropriately. The `0_` prefix on `singlelinesection.sk` ensures it loads first, which is a common pattern for dependency ordering in Skript. </blockquote></details> <details> <summary>tests/routines.sk (2)</summary><blockquote> `4-9`: **Test logic correctly validates routine execution patterns.** The test covers both direct Java constructor calls via async context and Skript function invocations through the fluent routine API. The assertions verify string representations appropriately. --- `12-13`: **Function definition is correct.** The comment clarifies the non-local requirement. The `object` return type is appropriate since `URL` is a Java type. </blockquote></details> <details> <summary>tests/functionsv2.sk (5)</summary><blockquote> `1-10`: **Supplier and getter tests provide good coverage.** Tests validate both lambda syntax and named supplier/getter constructs with various expression types. --- `12-19`: **Runnable test correctly validates side effects.** The pattern of clearing global state before running and asserting after is appropriate for side-effect verification. --- `22-55`: **Consumer, function, and bi-variant tests are thorough.** Coverage includes single-argument and bi-argument variants with appropriate state clearing between tests. --- `58-86`: **Repeated invocation and stream tests validate stateful behavior.** The consumer repeat test verifies accumulation across multiple calls. The Java stream test demonstrates interoperability with Java collections API. --- `89-107`: **Edge case and helper tests provide valuable coverage.** The negative test on line 96 correctly validates that real variable lists are rejected with `to fail` assertion. Lambda helper tests verify the `run lambda` utility syntax. </blockquote></details> <details> <summary>docs/testframework.md (3)</summary><blockquote> `5-56`: **Documentation structure and content are well-organized.** The overview, design goals, and feature parity sections clearly establish the framework's purpose and scope. The table format for feature parity is effective for quick reference. --- `155-214`: **Assertion and error tracking documentation is comprehensive.** The syntax variants, modifiers, and behavior descriptions provide sufficient detail for users to implement tests correctly. --- `216-287`: **Autorun semantics are clearly documented.** The distinction between autorun and manual execution modes is well-explained with practical usage patterns. </blockquote></details> <details> <summary>scripts/utils/testframework.sk (1)</summary><blockquote> `54-85`: **Test execution flow and cleanup look solid.** Clearing per-test errors, tracking failures, and restoring the test block helps keep runs isolated. </blockquote></details> <details> <summary>tests/singlinesection.sk (1)</summary><blockquote> `4-27`: **Assertions now validate concrete values.** The updated checks are clearer and less dependent on error-string matching. </blockquote></details> </blockquote></details> <sub>✏️ Tip: You can disable this entire section by setting `review_details` to `false` in your review settings.</sub> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Fetch the action.yml from devdinc/skript-test-action
curl -sL "https://api.github.com/repos/devdinc/skript-test-action/contents/action.yml?ref=v1.3" | jq -r '.content' 2>/dev/null | base64 -d 2>/dev/null | head -50Repository: devdinc/skripts
Length of output: 1148
🏁 Script executed:
# Also check the repository to see if the action needs write permissions
curl -sL "https://raw.githubusercontent.com/devdinc/skript-test-action/v1.3/action.yml" | head -60Repository: devdinc/skripts
Length of output: 1148
🏁 Script executed:
# Check the workflow file to see what operations are being performed
git ls-files | grep -E "test-skripts\.yml"Repository: devdinc/skripts
Length of output: 93
🏁 Script executed:
# Get the full workflow file to examine all steps
cat .github/workflows/test-skripts.yml | head -100Repository: devdinc/skripts
Length of output: 2467
Reduce permissions scope from contents: write to contents: read.
The workflow only performs read operations on the repository and does not write any changes back. The test action (devdinc/skript-test-action@v1.3) runs via Docker and does not declare permission requirements. Local filesystem operations like mkdir and rsync operate within the working directory and do not require repository write access. Use contents: read to follow the principle of least privilege.
| - name: Run tests | ||
| uses: devdinc/skript-test-action@v1.3 | ||
| with: | ||
| skript_repo_url: https://github.com/devdinc/Skript.git | ||
| # directory where your test scripts are located (relative to repo root) | ||
| test_script_directory: tests | ||
|
|
||
| # Skript version or ref (tag, branch, or commit) | ||
| skript_repo_ref: copy-dir-resources # 2.13.2 | ||
|
|
||
| # directory containing addon/plugin jars (relative to repo root) | ||
| extra_plugins_directory: build/libs | ||
|
|
||
| run_vanilla_tests: false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Attempt to check the action repository via web (without gh CLI authentication requirement)
# First, check if we can reach GitHub
curl -s -I https://github.com/devdinc/skript-test-action 2>&1 | head -5
echo "---"
# Try to fetch the release info
curl -s https://api.github.com/repos/devdinc/skript-test-action/releases/tags/v1.3 2>&1 | head -20Repository: devdinc/skripts
Length of output: 1466
🌐 Web query:
devdinc/skript-test-action GitHub repository action
💡 Result:
I couldn't find a GitHub repository or published Action named "devdinc/skript-test-action" on GitHub. Please check the repository name or share a link — I can search again or inspect the repo if you provide the URL.
🏁 Script executed:
#!/bin/bash
# Check if action.yml exists in the repository
curl -s https://raw.githubusercontent.com/devdinc/skript-test-action/main/action.yml 2>&1 | head -50
echo "---"
# Also try master branch in case
curl -s https://raw.githubusercontent.com/devdinc/skript-test-action/master/action.yml 2>&1 | head -50Repository: devdinc/skripts
Length of output: 1954
Remove the undefined skript_repo_url parameter.
The external action devdinc/skript-test-action@v1.3 exists and is properly configured for most parameters. However, skript_repo_url is not an accepted input by this action and will be silently ignored. The action accepts only: test_script_directory, skript_repo_ref, run_vanilla_tests, and extra_plugins_directory. Remove the skript_repo_url line or verify with the action maintainer if this parameter serves a different purpose.
| # Skript version or ref (tag, branch, or commit) | ||
| skript_repo_ref: copy-dir-resources # 2.13.2 |
There was a problem hiding this comment.
Branch ref may cause non-reproducible builds.
Using branch name copy-dir-resources instead of a tagged version (e.g., 2.13.2) means the workflow behavior can change unexpectedly when the branch is updated. Consider pinning to a specific commit SHA or tagged release for reproducibility.
| Below is a **single, unified Markdown document** that combines both inputs, removes duplication, and reconciles terminology while preserving all technical detail. Structure has been normalized so it reads as one authoritative specification rather than two overlapping documents. | ||
|
|
||
| ## Purpose | ||
| --- |
There was a problem hiding this comment.
Remove meta-commentary from document.
Line 1 contains editorial meta-text describing the document creation process. This should be removed from the final specification.
Suggested fix
-Below is a **single, unified Markdown document** that combines both inputs, removes duplication, and reconciles terminology while preserving all technical detail. Structure has been normalized so it reads as one authoritative specification rather than two overlapping documents.
-
---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Below is a **single, unified Markdown document** that combines both inputs, removes duplication, and reconciles terminology while preserving all technical detail. Structure has been normalized so it reads as one authoritative specification rather than two overlapping documents. | |
| ## Purpose | |
| --- | |
| --- |
| parse: | ||
| set {_raw} to first element of regex-1 | ||
|
|
||
| set {_parser} to ParserInstance.get() | ||
| set {_parser}.[ParserInstance]isActive to true | ||
| set {_backup} to {_parser}.backup() | ||
|
|
||
| {_parser}.setCurrentScript(current script) | ||
| {_parser}.setCurrentEvent("assert condition", (custom event "skriptTest").getClass()) | ||
|
|
||
| set {_cond} to Condition.parse({_raw}, "Can't understand condition: " + {_raw}) | ||
|
|
||
| set {_parser}.[ParserInstance]isActive to false | ||
| {_parser}.restoreBackup({_backup}) | ||
| continue | ||
| trigger: | ||
| set {_ok} to {_cond}.check(event) | ||
|
|
||
| set {_label} to "assert true" if parse tags contains "1" else "assert false" | ||
| set {_test} to event.getEventValue("string") | ||
| if all: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f testframework.skRepository: devdinc/skripts
Length of output: 89
🏁 Script executed:
cat -n scripts/utils/testframework.sk | head -150Repository: devdinc/skripts
Length of output: 6869
🏁 Script executed:
cat -n scripts/utils/testframework.sk | sed -n '125,135p'Repository: devdinc/skripts
Length of output: 636
Guard against null condition parsing result before invoking check().
The assert effect at line 103 parses a condition without guarding against null, but line 109 immediately calls .check(event) on a potentially null result. This differs from the event check block (lines 22-27) which properly guards: if {_cond} is set: {_cond}.check(event). If parsing fails, a NullPointerException masks the actual parse error. Add the guard and record the failure early.
Proposed fix
trigger:
- set {_ok} to {_cond}.check(event)
-
- set {_label} to "assert true" if parse tags contains "1" else "assert false"
set {_test} to event.getEventValue("string")
+ if {_cond} is not set:
+ testFail "Test ""%{_test}%"" has an invalid assert condition ""%{_raw}%"""
+ add "invalid condition: %{_raw}%" to {-test.sk::errors::%{_test}%::*}
+ if parse tags contains "3":
+ delay effect
+ stop
+ set {_ok} to {_cond}.check(event)
+ set {_label} to "assert true" if parse tags contains "1" else "assert false"| plural expression test errors[ for [test[s]] %-strings%]: | ||
| return type: strings | ||
| get: | ||
| if expr-1 is not set: | ||
| set {_test} to event.getEventValue("string") | ||
| return {-test.sk::errors::%{_test}%::*} | ||
| else: | ||
| loop ...expr-1: | ||
| set {_test} to loop-value | ||
| add {-test.sk::errors::%{_test}%::*} to {_r::*} | ||
| return {_r::*} |
There was a problem hiding this comment.
Reset the accumulator before building the error list.
{_r::*} can retain values across multiple calls in the same event. Clearing avoids cross-test contamination.
Proposed fix
get:
+ delete {_r::*}
if expr-1 is not set:
set {_test} to event.getEventValue("string")
return {-test.sk::errors::%{_test}%::*}
else:
loop ...expr-1:
set {_test} to loop-value
add {-test.sk::errors::%{_test}%::*} to {_r::*}
return {_r::*}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plural expression test errors[ for [test[s]] %-strings%]: | |
| return type: strings | |
| get: | |
| if expr-1 is not set: | |
| set {_test} to event.getEventValue("string") | |
| return {-test.sk::errors::%{_test}%::*} | |
| else: | |
| loop ...expr-1: | |
| set {_test} to loop-value | |
| add {-test.sk::errors::%{_test}%::*} to {_r::*} | |
| return {_r::*} | |
| plural expression test errors[ for [test[s]] %-strings%]: | |
| return type: strings | |
| get: | |
| delete {_r::*} | |
| if expr-1 is not set: | |
| set {_test} to event.getEventValue("string") | |
| return {-test.sk::errors::%{_test}%::*} | |
| else: | |
| loop ...expr-1: | |
| set {_test} to loop-value | |
| add {-test.sk::errors::%{_test}%::*} to {_r::*} | |
| return {_r::*} |
Changes
Project Structure
scripts/libs/andscripts/utils/directoriestest/totests/directoryscripts/utils/testframework.skCI/CD
test-skripts.yml)stableanddevbranchesstablebranchTesting Framework Enhancements
Test Updates
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.