diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8419060..d3cafce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,33 +1,69 @@ name: ci # pull_request only. A push trigger alongside it ran the whole suite twice on the same -# SHA, and the duplicate integration leg raced the other run against the Stream app -# several SDK repos share. The trade is that a merge to master, and a direct push to it, -# now run nothing: release.yml runs this same workflow only when a release is pending, -# so the gate before a tag is covered but the routine post-merge signal is gone. +# SHA. A merge to master runs nothing here: release.yml runs the unit lane only when a +# release is pending, so the gate before a tag is covered. on: pull_request: +# Needed at this level, not only in run_tests.yml: cancelling the reusable workflow's job +# does not cancel this run, so tests-passed would still run, read `cancelled` and publish a +# red aggregator for a push that has already been superseded. concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} cancel-in-progress: true permissions: contents: read - pull-requests: read jobs: - check-pr-title: - name: Validate PR title + # Skipped on a Release PR: it only bumps the version and rewrites the changelog. Not + # keyed on github.actor, which is the pusher: clicking Update branch reattributes the + # merge commit to whoever clicked, and the skip would stop firing on the normal release + # path. The author and head repo clauses are what make it unforgeable; the branch name + # on its own would let any PR, a fork's included, call its branch release-please--x. + unit: + if: >- + ${{ !(github.event.pull_request.user.login == 'github-actions[bot]' + && github.event.pull_request.head.repo.full_name == github.repository + && startsWith(github.head_ref, 'release-please--')) }} + uses: ./.github/workflows/run_tests.yml + + # The one required status check on master. A matrix job skipped by `if:` publishes a + # single check run with the template unexpanded, so per-leg contexts could never be + # satisfied on a Release PR; this job carries no matrix for that reason. `skipped` is + # accepted only when the condition above holds, repeated here because Actions cannot + # share an expression. `!cancelled()` rather than `always()`: a cancelled run must stay + # red, not report a pass. + tests-passed: + name: 🧪 Tests + needs: unit + if: ${{ !cancelled() }} runs-on: ubuntu-latest - permissions: - statuses: write + timeout-minutes: 5 steps: - - uses: aslafy-z/conventional-pr-title-action@v3 + - name: Check the unit lane env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - tests: - needs: check-pr-title - uses: ./.github/workflows/run_tests.yml - secrets: inherit + RESULT: ${{ needs.unit.result }} + RELEASE_PR: >- + ${{ github.event.pull_request.user.login == 'github-actions[bot]' + && github.event.pull_request.head.repo.full_name == github.repository + && startsWith(github.head_ref, 'release-please--') }} + run: | + case "$RESULT" in + success) + echo "unit lane passed" + ;; + skipped) + if [ "$RELEASE_PR" = "true" ]; then + echo "release pr: unit lane skipped by design" + else + echo "::error::the unit lane was skipped on a PR that is not a Release PR" + exit 1 + fi + ;; + *) + echo "::error::unit lane reported $RESULT" + exit 1 + ;; + esac diff --git a/.github/workflows/pr_title.yml b/.github/workflows/pr_title.yml new file mode 100644 index 0000000..923683d --- /dev/null +++ b/.github/workflows/pr_title.yml @@ -0,0 +1,17 @@ +name: Lint PR title + +on: + pull_request: + types: [opened, edited, reopened, synchronize] + +permissions: + pull-requests: read + +jobs: + pr_title: + name: 👮 Conventional PR title + runs-on: ubuntu-latest + steps: + - uses: amannn/action-semantic-pull-request@v6 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5389c44..ce748ab 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -165,7 +165,6 @@ jobs: needs: detect if: needs.detect.outputs.ready == 'true' uses: ./.github/workflows/run_tests.yml - secrets: inherit # Irreversible half. release: diff --git a/.github/workflows/run_integration.yml b/.github/workflows/run_integration.yml new file mode 100644 index 0000000..12b5776 --- /dev/null +++ b/.github/workflows/run_integration.yml @@ -0,0 +1,50 @@ +name: _run-integration + +on: + workflow_call: + secrets: + STREAM_API_SECRET: + # Lives in the ci environment, not at repo level, so it is not inheritable at + # call time; the job resolves it by declaring that environment below. + required: false + +concurrency: + # Repository-wide, on purpose. The contended resource is the Stream app, not the branch. + # Not cancel-in-progress: a half-run leaves users and channels behind. + group: run-integration + cancel-in-progress: false + +permissions: + contents: read + +jobs: + integration: + name: 🧪 Integration tests + environment: ci + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.1' + extensions: curl, json + tools: composer:v2 + + - name: Cache composer dependencies + uses: actions/cache@v4 + with: + path: vendor + key: composer-8.1-${{ hashFiles('composer.lock') }} + restore-keys: composer-8.1- + + - name: Install dependencies + run: composer install --prefer-dist --no-interaction + + - name: Integration tests + env: + STREAM_API_KEY: ${{ vars.STREAM_API_KEY }} + STREAM_API_SECRET: ${{ secrets.STREAM_API_SECRET }} + run: make test-integration diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index 7a7ded5..0d3163b 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -2,9 +2,6 @@ name: _run-tests on: workflow_call: - secrets: - STREAM_API_SECRET: - required: true concurrency: group: ${{ github.workflow }}-${{ github.ref }} @@ -13,27 +10,14 @@ concurrency: permissions: contents: read -# Every job below skips on a Release PR. release-please only bumps the version and rewrites -# the changelog, and every source commit in one already passed this suite on the PR it came -# from. The guard sits on each job rather than on the calling job in ci.yml, because a job -# skipped by `if:` reports success and satisfies a required status check, while a reusable -# workflow that is never called produces no check at all. It tests the author as well as the -# branch name: on its own, the name would let any PR, a fork's included, call its branch -# release-please--x and skip every required check, which branch protection counts as met. -# github.actor is the third clause, and it is the pusher rather than the PR author, so a -# human commit pushed onto the Release PR to fix a conflict or a changelog entry is tested -# like any other commit instead of riding the skip into main untested. +# The unit lane declares no environment and receives no credentials. Keep it that way: a +# fork PR gets no secrets and still goes green, and a live test added without the +# `integration` group fails here instead of passing against someone else's Stream app. jobs: test: - if: >- - ${{ !(github.actor == 'github-actions[bot]' - && github.event.pull_request.user.login == 'github-actions[bot]' - && github.event.pull_request.head.repo.full_name == github.repository - && startsWith(github.head_ref, 'release-please--')) }} - name: 🧪 Test & lint - environment: ci + name: 🧪 Test & lint (${{ matrix.php-version }}) runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 20 strategy: matrix: @@ -60,16 +44,6 @@ jobs: run: composer install --prefer-dist --no-interaction - name: Lint & unit tests - env: - STREAM_API_KEY: ${{ vars.STREAM_API_KEY }} - STREAM_API_SECRET: ${{ secrets.STREAM_API_SECRET }} run: | make lint make test-unit - - - name: Integration tests - if: matrix.php-version == '8.1' - env: - STREAM_API_KEY: ${{ vars.STREAM_API_KEY }} - STREAM_API_SECRET: ${{ secrets.STREAM_API_SECRET }} - run: make test-integration diff --git a/.github/workflows/scheduled_test.yml b/.github/workflows/scheduled_test.yml new file mode 100644 index 0000000..a1ab43b --- /dev/null +++ b/.github/workflows/scheduled_test.yml @@ -0,0 +1,51 @@ +name: Scheduled tests +# The integration lane's home. Advisory: see README.md. +on: + schedule: + # One hour per SDK repo sharing the Stream app, so no two mutate app-global settings at once. + - cron: "0 12 * * *" + workflow_dispatch: + +permissions: + contents: read + +jobs: + integration: + uses: ./.github/workflows/run_integration.yml + secrets: inherit + + # A failure here lands on no PR, and GitHub emails it only to whoever last edited the cron. + # Not failure(): a job that hits timeout-minutes concludes `cancelled`, and a hung run + # must report too. + report: + name: Report a red run + needs: integration + if: ${{ !cancelled() && needs.integration.result != 'success' }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + issues: write + steps: + - name: Open or update the tracking issue + env: + GH_TOKEN: ${{ github.token }} + TITLE: Daily integration run is red + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + # Unguarded, a non-2xx would abort the step under `bash -e` and a red run would + # report nothing at all. Fail toward creating the issue, which is the loud way. + if ! num="$(gh issue list --repo "$GITHUB_REPOSITORY" --state open --search "in:title \"$TITLE\"" \ + --json number,title --jq "map(select(.title == \"$TITLE\")) | .[0].number // empty")"; then + echo "::warning::Could not list open issues; creating a new one." + num="" + fi + if [ -n "$num" ]; then + gh issue comment "$num" --repo "$GITHUB_REPOSITORY" --body "Still red: $RUN_URL" + else + gh issue create --repo "$GITHUB_REPOSITORY" --title "$TITLE" --body \ + "The daily \`make test-integration\` run failed: $RUN_URL + + Integration is advisory, so nothing is blocked by this. It still has to be read: the suite + runs against a live Stream app shared by five SDK repos, so decide whether this is the app, + the backend, or this SDK, and close the issue once a daily run is green again." + fi diff --git a/Makefile b/Makefile index 840200b..634c832 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ test-unit: ## Run unit tests only ./vendor/bin/phpunit tests --exclude-group integration test-integration: ## Run integration tests in parallel (8 workers, method-level) - ./vendor/bin/paratest --processes=8 --runner=WrapperRunner --colors tests/Integration/ + ./vendor/bin/paratest --processes=8 --runner=WrapperRunner --colors --group integration tests test-specific: ## Run a specific test (usage: make test-specific TEST=TestClassName::testMethodName) ./vendor/bin/phpunit --filter $(TEST) diff --git a/README.md b/README.md index 4441acb..da4d75e 100644 --- a/README.md +++ b/README.md @@ -131,6 +131,16 @@ make test-unit make test-integration ``` +A test in the `integration` group talks to a live Stream app and needs credentials. `make test-unit` excludes that group and needs none; `make test-integration` runs only that group. Both select by group, so every test lands in exactly one lane. + +CI follows the same split: + +| When | What runs | Gates | +| --- | --- | --- | +| Pull request | `make lint` and `make test-unit` on PHP 8.1 to 8.3 | yes, `🧪 Tests` | +| Daily at 12:00 UTC | `make test-integration` | no, a red run opens an issue | +| Push to `master` with a release pending | the unit lane | yes, it gates the tag | + ## Usage ### Basic Setup @@ -234,14 +244,19 @@ Releases are driven by [release-please](https://github.com/googleapis/release-pl (`chore`, `ci`, `docs`, `test`, `refactor`) ship nothing. Keep the ticket prefix after the type, as in `feat: [FEEDS-1350] add feed retention endpoint`. - release-please keeps a Release PR open with the version bump in `composer.json`, - `src/Constant.php` and `CHANGELOG.md`. It is opened by `github-actions[bot]`, so - approve it and run its held checks like any other PR. Never edit those versions by - hand; the `// x-release-please-version` comment in `src/Constant.php` is what the - updater anchors on. -- Merging the Release PR runs lint, unit tests across PHP 8.1 to 8.3 and the integration - suite on that merge commit, which is the commit the tag will point at. Only if that is - green does the workflow create the tag and the GitHub Release and announce the tag to - Packagist. The order matters: a tag and a GitHub Release cannot be withdrawn. + `src/Constant.php` and `CHANGELOG.md`. Never edit those versions by hand; the + `// x-release-please-version` comment in `src/Constant.php` is what the updater + anchors on. +- Its runs are created held at `action_required` until someone clicks **Approve and + run**, because release-please opens the PR with `GITHUB_TOKEN`. The unit lane then + reports `skipped` and `🧪 Tests` goes green without running a test. The skip keys on + the PR author, so a commit pushed onto a Release PR by hand is skipped too and reaches + `master` untested. +- Merging the Release PR runs lint and unit tests across PHP 8.1 to 8.3 on that merge + commit, which is the commit the tag will point at. Only if that is green does the + workflow create the tag and the GitHub Release and announce the tag to Packagist. The + order matters: a tag and a GitHub Release cannot be withdrawn. Integration tests are + advisory and gate none of it. Packagist reads the tags itself, so the announce step only asks it to look now rather than on its own schedule. If it fails or the credentials are unset, the release still diff --git a/tests/ClientBuilderTest.php b/tests/ClientBuilderTest.php index f76a97a..907062e 100644 --- a/tests/ClientBuilderTest.php +++ b/tests/ClientBuilderTest.php @@ -25,6 +25,11 @@ protected function setUp(): void unset($_ENV['STREAM_API_KEY'], $_ENV['STREAM_API_SECRET'], $_ENV['STREAM_BASE_URL']); } + protected function tearDown(): void + { + unset($_ENV['STREAM_API_KEY'], $_ENV['STREAM_API_SECRET'], $_ENV['STREAM_BASE_URL']); + } + /** * @test */ @@ -119,12 +124,13 @@ public function explicitCredentialsOverrideEnvironment(): void public function buildRequiresApiKey(): void { // Test that providing API secret but no API key still works if key is in environment + $_ENV['STREAM_API_KEY'] = 'env-key'; $client = (new ClientBuilder()) ->apiSecret('test-secret') ->build(); self::assertInstanceOf(Client::class, $client); - self::assertNotEmpty($client->getApiKey()); // Should get from environment + self::assertSame('env-key', $client->getApiKey()); self::assertSame('test-secret', $client->getApiSecret()); } @@ -134,13 +140,14 @@ public function buildRequiresApiKey(): void public function buildRequiresApiSecret(): void { // Test that providing API key but no API secret still works if secret is in environment + $_ENV['STREAM_API_SECRET'] = 'env-secret'; $client = (new ClientBuilder()) ->apiKey('test-key') ->build(); self::assertInstanceOf(Client::class, $client); self::assertSame('test-key', $client->getApiKey()); - self::assertNotEmpty($client->getApiSecret()); // Should get from environment + self::assertSame('env-secret', $client->getApiSecret()); } /** diff --git a/tests/ClientTest.php b/tests/ClientTest.php index cbf8281..b443cdb 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -20,6 +20,15 @@ class ClientTest extends TestCase protected function setUp(): void { $this->mockHttpClient = $this->createMock(HttpClientInterface::class); + + // Set rather than read the ambient environment: CI runs this lane with no credentials. + $_ENV['STREAM_API_KEY'] = 'env-key'; + $_ENV['STREAM_API_SECRET'] = '0123456789abcdef0123456789abcdef'; + } + + protected function tearDown(): void + { + unset($_ENV['STREAM_API_KEY'], $_ENV['STREAM_API_SECRET']); } /** diff --git a/tests/Integration/ConnectionPoolLogTest.php b/tests/Http/ConnectionPoolLogTest.php similarity index 97% rename from tests/Integration/ConnectionPoolLogTest.php rename to tests/Http/ConnectionPoolLogTest.php index 88f8146..72439c4 100644 --- a/tests/Integration/ConnectionPoolLogTest.php +++ b/tests/Http/ConnectionPoolLogTest.php @@ -2,15 +2,12 @@ declare(strict_types=1); -namespace GetStream\Tests\Integration; +namespace GetStream\Tests\Http; use GetStream\ClientBuilder; use PHPUnit\Framework\TestCase; use Psr\Log\AbstractLogger; -/** - * @group integration - */ class ConnectionPoolLogTest extends TestCase { public function testClientInitializedLogContainsAllKnobs(): void