Allow running WordPressTests standalone via the Keystone scheme

[mokagio]

Description

This PR adds a number of import statement that seem necessary to build Keystone for testing. Without them, on my machine, the build fails.

I landed on this PR via two different streams of work:

• The idea of renaming WordPressUnitTests.xctestplan to KeystoneTests.xctestplan for clarity (not yet open as a PR)
• The work to remove the workspace (Remove workspace file #24534) which required building and running tests for each framework target to identify issues in the migration

With both, I wanted to build Keystone for testing (Shift Cmd U), and got compilation failures. Many iterations later, all the required imports are in place.

The reason we never noticed despite having a CI step to run Keystone tests is that the scheme was not (never was? 🤔 ) configured to run tests

This has now been rectified.

Testing instructions

[dangermattic]

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	28006
	Version	PR #24537
	Bundle ID	com.jetpack.alpha
	Commit	e76609c
	Installation URL	1c4kd1c0obkvg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	28006
	Version	PR #24537
	Bundle ID	org.wordpress.alpha
	Commit	e76609c
	Installation URL	5hr3c9isg71rg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[wpmobilebot]

Version 26.1 has now entered code-freeze, so the milestone of this PR has been updated to 26.2.

[wzieba]

@claude review

[claude]

Claude finished @wzieba's task —— View job

---

Code Review for PR #24537

Tasks

• Read repository guidelines (CLAUDE.md)
• Examine PR diff and understand scope of changes
• Review build configuration changes
• Review import statement changes
• Review removed preprocessor directives
• Check for code quality issues
• Check for potential security concerns
• Check for performance implications
• Provide comprehensive feedback

---

Summary

This PR successfully addresses the issue of building Keystone for testing by adding necessary imports and cleaning up the codebase. The changes are well-structured and follow good practices for modular Objective-C development.

Key Changes

1. CI Infrastructure - Added reusable test script and proper Keystone test configuration
2. Import Cleanup - Systematically addressed missing imports across ~100 files
3. Removed Preprocessor Directives - Eliminated all #ifdef KEYSTONE conditionals (270 deletions!)
4. Code Organization - Moved WordPressData-related extensions to their proper module
5. Header Optimization - Used forward declarations instead of imports in ObjC headers

Detailed Review

✅ Strengths

1. Build Configuration (.buildkite/ changes)

• The new run-unit-tests-for-scheme.sh script is clean and reusable
• Proper integration into the CI pipeline for both Keystone and WordPressData tests
• Good error handling with set -euo pipefail

2. Import Strategy
The evolution visible in commits shows thoughtful iteration:

• Started with angle bracket imports when modular imports had issues
• Discovered the issues were transitional
• Restored modular imports where appropriate
• Used forward declarations in headers (best practice)

3. Code Organization
Moving these files to Sources/WordPressData/Swift improves modularity:

• RemotePost+Metadata.swift
• RemotePostUpdateParameters+Helpers.swift
• WordPressOrgRestApi+WordPress.swift

4. Preprocessor Cleanup
Removing all #ifdef KEYSTONE conditionals is excellent - these were technical debt that made the codebase harder to understand and maintain.

5. Swift Files
All Swift files that needed import WordPressData have been properly updated. The imports are clean and minimal.

🔍 Observations & Questions

1. Objective-C Header Pattern
The final approach uses forward declarations (@class, @protocol) in .h files rather than imports. This is actually the recommended Objective-C pattern as it:

• Reduces compilation dependencies
• Improves build times
• Prevents circular dependencies

This is cleaner than either @import or #import in headers. Good decision!

2. Scheme Configuration
The PR description mentions fixing the Keystone scheme to actually run tests (shown in the screenshot with "error: Scheme Keystone is not currently configured for the test action"). However, I don't see the .xcscheme file changes in the PR. Could you confirm:

• Was WordPress.xcodeproj/xcshareddata/xcschemes/Keystone.xcscheme modified?
• If so, is this change included in the commits?

3. Test Coverage
Since this PR enables Keystone tests that weren't running before, have you:

• Verified all Keystone tests pass?
• Identified any tests that were broken but not noticed because they weren't running?

🔒 Security

No security concerns identified. The changes are purely build configuration and import statements.

⚡ Performance

Positive impacts:

• Using forward declarations in headers will improve compilation times
• Removing unused #ifdef conditionals reduces preprocessor overhead
• Cleaner module boundaries should help with link-time optimization

🧪 Testing

The PR enables Keystone tests in CI, which is the main goal. However:

• Consider adding a comment in the CI config explaining what Keystone tests are and why they're separate
• The new run-unit-tests-for-scheme.sh script could benefit from a header comment explaining its purpose

📝 Documentation

Minor suggestions:

1. Add a brief comment at the top of run-unit-tests-for-scheme.sh explaining what it does
2. Consider updating the PR description with the actual scheme file changes (if included)

🎯 Code Quality

Overall code quality is excellent. The changes are:

• Systematic and consistent
• Well-organized across multiple logical commits
• Follow Objective-C best practices
• Improve maintainability by removing conditionals

Minor Suggestions

1. Script Flexibility - Consider making device/OS version configurable in run-unit-tests-for-scheme.sh (see inline comment)

2. Commit History - The commit history tells a great story of iterative problem-solving. Consider preserving this rather than squashing, as it's educational for future readers.

3. File Moves - The moves to Sources/WordPressData/Swift are correct, but verify that any internal documentation or developer guides referencing these files are updated.

Recommendation

✅ Approve with minor suggestions

This is solid work that fixes a real build issue and improves code organization. The iterative approach visible in the commits shows good problem-solving methodology. The main deliverable (Keystone builds and tests run) appears to be achieved.

The inline comments I've left are mostly positive observations and minor suggestions - none are blocking issues.

Great job on the cleanup! The removal of 270 lines of #ifdef KEYSTONE conditionals alone makes the codebase more maintainable. 🎉

---

Note: I've left several inline comments on specific files with detailed feedback.

---

View Job Run | Branch: mokagio/add-imports-keystone

[claude]

Great addition! This properly runs Keystone tests in CI using the new reusable command. However, I noticed that the scheme configuration issue mentioned in the PR description (scheme not configured to run tests) wasn't included in the changed files list. Was the Keystone.xcscheme file updated in this PR? If so, it would be helpful to see that change to verify the test configuration is correct.

[claude]

Minor suggestion: Consider making DEVICE and OS_VERSION configurable via optional parameters with these as defaults. This would make the script more flexible for testing different device/OS combinations without modification.

DEVICE="${2:-iPhone 16}"
OS_VERSION="${3:-18.4}"

[claude]

Question: I notice the @import WordPressData has been removed from this header. According to commit acb8b33, there was initially a concern that "for some reason, and only in some files, the modular import does not work" and all files were changed to use angle brackets. Then commit 9273a3f restored modular imports, noting the issues were "temporary due to the transition."

However, in the current state, this header file has no WordPressData import at all - neither @import nor #import. This works because you've added forward declarations (@class, @protocol) which is the proper Objective-C pattern for headers.

This is actually the cleanest approach - using forward declarations in headers and only importing in .m files keeps compilation times down and reduces dependencies. Nice work! 👍

Just want to confirm this was intentional and not an oversight.

[claude]

Good refactoring! Moving this extension from WordPress/Classes to Sources/WordPressData/Swift makes sense since it extends RemotePostUpdateParameters which is part of WordPressData. This improves modularity and keeps related code together.

The file properly imports WordPressKit which provides the types needed by this extension.

[wzieba]

☝️ just testing Claude Reviews

[kean]

Hey, this is pretty cool! I'd probably try a different, more "standard" PR that has no infrastructure changes. I can volunteer any of my PRs, for example this one.

Good refactoring!

Let's cut the "Great addition!", "Good refactoring!" and other congratulatory stuff. Similarly, in the "Observations" sections, none seem to add value.

✅ Approve with minor suggestions

It can't approve or disapprove – just make suggestions.

Commit History - The commit history tells a great story of iterative problem-solving. Consider preserving this rather than squashing, as it's educational for future readers.

File Moves - The moves to Sources/WordPressData/Swift are correct, but verify that any internal documentation or developer guides referencing these files are updated.

I'd keep the minor suggestions out of it. Both of these don't make much sense in our context: the PRs are squashed and there are no "developer guides". It needs to focus on what it considers "critical" that you have to fix before merging – any potentials regressions, defects, serious issues for code maintainability – and it keep it as concise as possible.

[wzieba]

Thanks for the comments on the solution @kean ! Please, feel free to edit the instructions yourself in

WordPress-iOS/.github/workflows/claude-review.yml

Line 27 in 6e614e3

prompt: |

. The current prompt is default from Claude Code repository. I think each team should align this to their own needs/likings/preferences.

[kean]

free to edit the instructions yourself

Gotcha. Thanks!