Skip to content

Create Tests Trait - #155

Merged
n7studios merged 3 commits into
masterfrom
tests-trait
Aug 18, 2026
Merged

Create Tests Trait#155
n7studios merged 3 commits into
masterfrom
tests-trait

Conversation

@n7studios

@n7studios n7studios commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Moves method tests to a new TestsTrait which can easily be shared with the Kit WordPress Libraries (PR here), in the same way the ConvertKit_API_Traits can be easily shared with the Kit WordPress Libraries.

Other methods and test classes are specific to the PHP SDK or WordPress Libraries.

This allows for easy copy/paste of the traits and their tests from the PHP SDK to the WordPress Libraries, without editing other files.

Testing

Existing tests pass.

Checklist

@n7studios n7studios self-assigned this Aug 10, 2026
@n7studios
n7studios requested review from a team, ciccio-kit and noelherrick and removed request for a team August 12, 2026 14:29
@n7studios
n7studios marked this pull request as ready for review August 13, 2026 08:02

@ciccio-kit ciccio-kit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

so this is a big PR but seems low risk and good to go from my point of view so I'm approving. I also gave it a run through Claude and it did report a few non blocking points. I'll paste the summarized report here, in case it provides any value to you:

  1. Fold the WP-side helper fix in before #127 merges. The two trait copies already differ in 25 lines, and three are the WP copy being ahead, so the next copy from here reverts them. The one that bites: assertDataExists/assertPaginationExists are is_object($result) ? get_object_vars($result) : $result in #127, because WP Libs decodes with json_decode(..., true) and passes arrays. Bare get_object_vars() raises TypeError: Argument #1 ($object) must be of type object, array given, and those two helpers have 152 and 151 call sites. The is_object() check is a no-op here. Same for CONVERTKIT_API_LANDING_PAGE_CHARACTER_ENCODING_URL (same page, slug cc5eb21744 — this repo's .env.example just never defined the name) and the get_purchases() → get_purchase() docblock at line 6580.

Worth deciding how you'd notice this next time: a composer script or CI step diffing the two files turns a silent revert into a red build. Right now the only safeguard is diffing by hand, and it's already slipped once.

  1. Two of the 71 conversions lose a real assertion. No objection to the loose form generally — ClientException appears nowhere in src/, and assertApiError still fails when nothing throws. But create_webhook and get_resource carry an explicit @throws \InvalidArgumentException (src/ConvertKit_API_Traits.php:2272, src/ConvertKit_API.php:276) and validate client-side before any HTTP call. testCreateWebhookWithInvalidEvent and testGetResourceInvalidURL were the only things checking that, and both would still pass if the validation vanished and the call went to the wire. An optional ?string $expected = null on the helper restores those two, leaves the other 69 call sites untouched, and #127's shim can ignore it.
  1. Minor. ConvertKitAPITest could be abstract. The in phpunit.xml only guards the directory scan, so phpunit tests/ConvertKitAPITest.php runs 258 tests against a null $this->api. Making it abstract lets you delete the exclude — discovery stays at 540.

@n7studios
n7studios merged commit fb4f8fa into master Aug 18, 2026
29 of 54 checks passed
@n7studios n7studios mentioned this pull request Aug 18, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants