Skip to content

test: add PHPUnit harness and CI matrix (no behavior changes)#157

Open
dokun1 wants to merge 11 commits intogharlan:mainfrom
dokun1:tests/add-phpunit-harness
Open

test: add PHPUnit harness and CI matrix (no behavior changes)#157
dokun1 wants to merge 11 commits intogharlan:mainfrom
dokun1:tests/add-phpunit-harness

Conversation

@dokun1
Copy link
Copy Markdown

@dokun1 dokun1 commented Apr 11, 2026

Summary

This PR introduces a test harness with zero behavior changes to the workflow itself. It adds PHPUnit as a dev dependency, a bootstrap + base test case for isolating Workflow's static state, six test files that pin the current single/dual-slot behavior, and a GitHub Actions matrix that runs the suite on every push and pull request.

No production files (workflow.php, curl.php, item.php, search.php, action.php, server.php, info.plist, .php-cs-fixer.dist.php) are touched. The only composer.json edit is adding phpunit/phpunit: ^11.0 to require-dev.

My motivation is a follow-up PR to add multi-account support (multiple github.com tokens + multiple enterprise instances with an active-account switcher). That change will touch token storage, request_cache partitioning, and the OAuth callback — areas where characterization tests would catch regressions early. I wanted to land the harness first so the multi-account PR can be reviewed against a green baseline. This PR is independently useful even if the multi-account work never lands — the tests document current behavior and protect future refactors.

What is pinned

File Characterizes
tests/WorkflowConfigTest.php setConfig / getConfig / removeConfig round-trip, defaults, overwrite
tests/WorkflowTokenTest.php access_token vs enterprise_access_token isolation, per-slot remove
tests/WorkflowSchemaTest.php config and request_cache column shapes, parent_url index, idempotent re-init
tests/WorkflowEnterpriseUrlTest.php baseUrl/apiUrl/gistUrl derivation from enterprise_url config, per-page query building
tests/CurlRequestTest.php CurlRequest constructor field assignment, nullable token/etag
tests/ItemRenderTest.php Item::toXml output — uid hashing, icon fallback, enterprise e prefixing, baseUrl arg rewrite, autocomplete rules, HTML escaping, valid/invalid suffixing

Total: 30 tests, 59 assertions, <50ms locally.

Test-harness implementation notes

  • tests/bootstrap.php chdirs into the repo root before requiring workflow.php, because workflow.php uses relative require 'item.php' / require 'curl.php'. Touching those requires would be a behavior change and is out of scope.
  • tests/WorkflowTestCase.php gives each test its own temp alfred_workflow_data directory and uses reflection to reset Workflow's static properties between tests. This is necessary because Workflow is a static class and without it the SQLite handle and prepared statements leak across tests.
  • Tests use the real SQLite backend (not a mock) so schema assertions match runtime behavior.
  • No tests hit the network. CurlRequest is tested as a value object only.

CI matrix

PHP 8.2 / 8.3 / 8.4 on ubuntu-latest, via shivammathur/setup-php@v2. No coverage, composer:v2, extensions pinned to what the workflow actually imports (pdo, pdo_sqlite, sqlite3, curl, simplexml).

I picked 8.2 as the floor because:

  • composer.json does not declare a minimum PHP version, so there's nothing authoritative to defer to.
  • Current source uses nullable-type syntax (e.g. ?Curl $curl = null in workflow.php) that requires PHP 8.0+.
  • 8.1 is EOL as of Dec 2025; 8.2 is the current oldest-supported upstream release.

Happy to adjust the matrix if you'd rather support older versions or drop 8.4 until it's more widely deployed.

What is explicitly NOT in this PR

  • No changes to any .php source file
  • No composer.lock (already gitignored by the project)
  • No refactors, renames, or code-style fixes
  • No behavior changes of any kind

If you'd prefer a narrower first cut (e.g. only the ItemRenderTest without the Workflow* tests, or without CI), I'm happy to split it — just let me know.

Test plan

  • vendor/bin/phpunit passes locally on PHP 8.5 (30/30)
  • CI passes on PHP 8.2 / 8.3 / 8.4
  • No production file modified (git diff upstream/main...HEAD -- '*.php' ':!tests/*' is empty)
  • Maintainer confirms CI matrix and approach

Copilot AI review requested due to automatic review settings April 11, 2026 19:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a PHPUnit-based characterization test harness and CI execution for the existing Alfred GitHub Workflow code, establishing a green baseline before future behavioral changes (e.g., multi-account support).

Changes:

  • Add PHPUnit dev dependency + composer test script, plus a phpunit.xml.dist configuration.
  • Introduce a bootstrap and base test case to isolate Workflow’s static state and per-test SQLite data dirs.
  • Add unit/characterization tests for Workflow, Item::toXml, and CurlRequest, and run them in a GitHub Actions PHP version matrix.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/bootstrap.php Loads production code for tests and provides helper functions to isolate/reset Workflow static state.
tests/WorkflowTestCase.php Base PHPUnit test case that sets per-test temp alfred_workflow_data and resets Workflow.
tests/WorkflowConfigTest.php Characterizes config set/get/remove behavior.
tests/WorkflowTokenTest.php Characterizes github vs enterprise token slot isolation/removal behavior.
tests/WorkflowSchemaTest.php Pins SQLite schema and idempotent init behavior.
tests/WorkflowEnterpriseUrlTest.php Pins base/api/gist URL derivation rules and API URL query building.
tests/CurlRequestTest.php Validates CurlRequest constructor field assignment and nullability.
tests/ItemRenderTest.php Characterizes Item::toXml output rules (escaping, prefixes, arg rewriting, ordering).
phpunit.xml.dist PHPUnit configuration (bootstrap, cache dir, strictness flags, suite discovery).
composer.json Adds phpunit/phpunit to require-dev and a test script.
.gitignore Ignores PHPUnit cache, vendor artifacts, and composer.phar.
.github/workflows/test.yml Runs the PHPUnit suite on push/PR across PHP 8.2–8.4.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/bootstrap.php
];
foreach ($resets as $name => $value) {
if ($ref->hasProperty($name)) {
$prop = $ref->getProperty($name);
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

agw_test_reset_workflow() uses reflection to reset Workflow's private static properties, but it never makes the ReflectionProperty accessible. On PHP 8.2+ this will throw when calling setValue() for private/protected properties, causing the entire test suite to fail. Set $prop->setAccessible(true) (or use another visibility-bypassing approach) before setting values.

Suggested change
$prop = $ref->getProperty($name);
$prop = $ref->getProperty($name);
$prop->setAccessible(true);

Copilot uses AI. Check for mistakes.
Comment thread tests/bootstrap.php
Comment on lines +5 to +6
chdir(__DIR__.'/..');
require __DIR__.'/../workflow.php';
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

tests/bootstrap.php changes the process working directory via chdir(__DIR__.'/..') and leaves it there for the rest of the PHPUnit run. This can make future tests/order-dependent behavior fragile (any relative path access will now resolve from repo root). Consider capturing the original CWD and restoring it after requiring workflow.php so bootstrap has minimal global side effects.

Suggested change
chdir(__DIR__.'/..');
require __DIR__.'/../workflow.php';
$originalCwd = getcwd();
chdir(__DIR__.'/..');
try {
require __DIR__.'/../workflow.php';
} finally {
if (false !== $originalCwd) {
chdir($originalCwd);
}
}

Copilot uses AI. Check for mistakes.
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.

2 participants