Skip to content

Conversation

@Fellmonkey
Copy link
Contributor

@Fellmonkey Fellmonkey commented Dec 2, 2025

What does this PR do?

This PR fixes multiple issues in the PHP SDK test templates that were causing significant test failures (TypeErrors and Assertion failures).

Specifically, it addresses the following:

  • Fixes Service Test TypeErrors: Updated ServiceTest.php.twig to correctly import and use Enum classes for method parameters instead of passing raw strings. This resolves the TypeErrors seen in AccountTest, UsersTest, etc., where arguments were expected to be specific Enum instances (e.g., Appwrite\Enums\AuthenticatorType).
  • Fixes Query Test Assertions: Updated QueryTest.php.twig to parse the returned JSON string from Query methods and assert against the structured data (method, attribute, values) instead of performing exact string matching. This fixes failures where the tests expected a function-like string (e.g., equal("attr", ["s"])) but received a JSON string.
  • Fixes ID Test: Updated IDTest.php.twig to verify the length of the generated unique ID (20 chars) instead of asserting equality with the literal string 'unique()'.
  • Updates API Template: Updated api.twig to correctly handle the location method type in the Client::call arguments.

Test Plan

regenerate sdk, run php tests
before:
{8CAF1924-A23F-4F7B-848D-7CF320A28E57}
after:
{25D56ECB-551A-43C9-82E9-37DB7748F4F7}

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you read the Contributing Guidelines on issues?

YES

Summary by CodeRabbit

Tests

  • Updated ID generation tests to validate dynamically generated IDs by length rather than fixed strings
  • Strengthened query builder test coverage with JSON-based validation across multiple operators
  • Enhanced service test framework to support enum types, array return values, and extended parameter handling

✏️ Tip: You can customize this high-level summary in your review settings.

Expanded api.twig to support 'location' method type. Enhanced IDTest.php.twig to check unique ID length. Refactored QueryTest.php.twig to validate JSON structure and content of Query methods instead of string output. Updated ServiceTest.php.twig to import enum types and use enum values for parameters, and to support additional method types in mocks.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

Three PHP test template files are being updated. The IDTest template now validates generated ID length rather than a fixed string. The QueryTest template replaces direct string assertions with JSON-based parsing and structural validation across multiple query operators. The ServiceTest template adds dynamic enum type imports based on method parameters, extends array return-type handling, adjusts mock setup for webAuth methods, and enhances parameter generation to output concrete enum values when applicable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Enum import logic in ServiceTest: Review the dynamic enum scanning and import generation logic to ensure all unique enum types are correctly identified and imported.
  • JSON parsing in QueryTest: Verify that all operator test cases (20+ operators) correctly parse and validate the JSON structure, including null-handling edge cases.
  • Mock setup conditional logic: Examine the webAuth type detection and the conditional Mockery::any() appending logic to ensure mock calls receive the correct argument count.
  • Parameter generation with enums: Confirm that enum parameter substitution correctly selects the first key/value and maintains backward compatibility with existing parameter types (objects, arrays, files, booleans, strings, integers).

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix php tests' is vague and overly generic, using non-descriptive language that doesn't convey the specific changes made. Use a more specific title that highlights the main changes, such as 'Update PHP test templates to handle enums and JSON parsing' or 'Fix PHP SDK test templates for enum handling and query assertions'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
templates/php/tests/Services/ServiceTest.php.twig (1)

9-19: Enum imports generation is sound; consider normalizing names for dedupe

The pre-pass over service.methods correctly emits use Appwrite\Enums\... only once per enumName, which should fix missing-import TypeErrors in generated tests.

If you ever end up with inconsistent casing for parameter.enumName across methods, you could normalize before checking added to avoid duplicate use lines, e.g. store parameter.enumName | caseUcfirst in a temp variable and use that for both the use and the added array. This is purely defensive and not required for current usage.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40389df and 924937c.

📒 Files selected for processing (1)
  • templates/php/tests/Services/ServiceTest.php.twig (2 hunks)
🔇 Additional comments (3)
templates/php/tests/Services/ServiceTest.php.twig (3)

42-52: Array return-type handling extension looks correct

Treating (method.responseModel == 'any') and (method | getReturn == 'array') as array responses and initializing $data = array(); keeps the test stubs aligned with actual service return types while preserving the structured-model and string fallbacks. This is a clean, targeted fix with no obvious side effects.


54-55: Conditional extra Mockery::any() for webAuth methods matches call signature

Extending the allows()->call(...) expectation to include a fifth Mockery::any() when method.type == 'webAuth' makes the mock signature align with the client’s call usage for those methods, fixing argument-count mismatches while keeping non‑webAuth methods unchanged.


57-59: Enum parameter emission now matches official PHP SDK usage

Switching enum-typed parameters to {{ EnumName | caseUcfirst }}::{{ ... | caseEnumKey }}() aligns generated tests with the documented PHP SDK style (use Appwrite\Enums\AuthenticatorType; ... AuthenticatorType::TOTP()). (appwrite.io)

This should resolve prior TypeErrors where raw strings were passed instead of enum instances, while leaving all non-enum parameter cases (objects, arrays, files, scalars) unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant