Skip to content

Use test-actions as the standard place to test the automations - #99

Merged
rtibbles merged 10 commits into
mainfrom
test-actions-testing-flow
Sep 25, 2026
Merged

rtibbles merged 10 commits into
mainfrom
test-actions-testing-flow

Conversation

@akolson

@akolson akolson commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Summary

Testing a change to the automations, or to the sync workflow, meant setting up a separate GitHub
organization with its own app, secrets, and Google service account.

learningequality/test-actions already has all of that. This makes it the standard place to test,
so a developer needs no setup.

The change

  • scripts/sync-automation-template.js takes --only <repo>, exposed as an only input on the
    workflow. A run then covers one repo and cannot reach another, so containment is an input rather
    than a token the developer has to build.
  • docs/testing-automations.md covers testing an automation, testing the sync, and testing a change
    to a reusable workflow. Linked from docs/automation.md.
  • docs/testing-in-a-separate-org.md carries the setup steps for the rarer route, so the page a
    developer reads first stays short.

The org listing is skipped entirely when only is set, rather than filtered afterwards, so there is
no path to the other repos. A test fails if that listing is reached.

Outside this repo, test-actions lost six callers pointing at MisRob/.github on a feature branch,
and gained an unedited copy of automation-template.yml. It is now a consumer like any other, and
holds repository secrets pointing the spreadsheet at a test sheet rather than the production one.

References

Closes #98

Reviewer guidance

Verified against the live org: --only=test-actions reports one repo, and a full run reports all
nine including test-actions. 71 tests pass, along with the generator drift check and every hook.

  1. Run GITHUB_TOKEN=$(gh auth token) node scripts/sync-automation-template.js --dry-run --only=test-actions. Expect one row and no other repo.
  2. Run it without --only. Expect nine rows, all in-sync.
  3. Read docs/testing-automations.md as someone testing a change for the first time. The caveat
    worth checking is the one about is-contributor, since automations gated on it skip for anyone
    in the organization, so triggering those needs a second account outside it.

AI usage

I used Claude Code to add the flag and its test, to prepare test-actions, and to draft the docs
and this description. I decided the approach, chose to keep the spreadsheet ids unset by default
rather than inheriting production, and verified both run shapes against the live org.

Testing the write path meant a token scoped to repos the developer owned, which
is why testing needed a separate organization at all. With --only, and the
matching workflow input, a run covers one repo and cannot reach another, so
containment is an input rather than a credential someone has to build.

The org listing is skipped entirely when only is set, rather than filtered
afterwards, so there is no path to the other repos. A test fails if that
listing is reached.

docs/testing-automations.md describes both flows in test-actions, where the bot
app and the secrets are already in place.
Several automations gate on is-contributor, so they skip for anyone in the
organization. Testing them in test-actions therefore needs a second GitHub
account that is not a member, which needs no permissions, secrets or app of its
own. For a pull request it forks and opens one back, and pull_request_target
runs in test-actions with test-actions' secrets rather than the fork's.
The extension adds nothing a reader needs, and both internal links now read the
same way.
The page said only that testing against a non-production app needs another
organization, which glossed over registering an app, installing it, and
creating a service account and a spreadsheet shared with it.

Those steps now have their own page, linked from the testing page, along with
the three cases where the route is worth the setup. Keeping them separate stops
the longest instructions belonging to the least common path from dominating the
page a developer reads first.
A filename in the middle of a sentence tells a reader less than the title does,
and reads worse.
@akolson
akolson requested a review from rtibblesbot September 25, 2026 09:35
@rtibblesbot

rtibblesbot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Waiting for changes

Last updated: 2026-09-25 12:51 UTC

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR #99: --only <repo> (the issue's own spelling) is silently ignored, so the sync runs against the whole org. Otherwise --only works, is tested, and is exposed in the workflow. Docs cover both test flows.

CI passes. 34 tests pass locally. No UI changes. I could not verify the test-sheet secrets on test-actions.

  • suggestion: --only space form falls through to a full-org run (inline)
  • suggestion: workflow interpolates inputs.only straight into the shell (inline)
  • suggestion: excluded only target reports "0 consumers" silently (inline)
  • suggestion: separate-org page is beyond the issue's deliverables (inline)
  • suggestion: say how to undo the test drift (inline)
  • nitpick: unrelated link rewording in community-automations.md (inline)

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran a phased review pipeline over the pull request diff:

  • Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
  • Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
  • Specialized frontend/backend review passes applied framework-specific lenses where those files changed
  • For UI changes: manual QA and an accessibility audit against a live dev server, when available
  • Checked CI status and linked issue acceptance criteria
  • Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence

Comment thread scripts/sync-automation-template.js Outdated
}
const template = fs.readFileSync(TEMPLATE_PATH, 'utf8');
const results = await run(httpApi(token), template, { dryRun: process.argv.includes('--dry-run') });
const onlyArg = process.argv.find((a) => a.startsWith('--only='));

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: --only test-actions (the form in #98) is silently ignored, so the sync runs against the whole org and opens real PRs. Only the --only= form is parsed. --only= with no value falls through the same way. Please accept both forms, or exit non-zero when an --only argument yields no repo name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in cad6b69. Both forms parse, and an --only with no repo name exits 1 rather than running against everything. Confirmed live: --only test-actions reports one repo, --only alone reports "--only needs a repo name".

run: |
node scripts/sync-automation-template.js \
${{ inputs.dry_run && '--dry-run' || '' }} \
${{ inputs.only && format('--only={0}', inputs.only) || '' }}

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: inputs.only is expanded directly into the shell of a step that holds an org-wide write token. Dispatch requires write access, so the risk is low. Please pass it via env: (ONLY: ${{ inputs.only }}) and use "--only=$ONLY", per GitHub's hardening guidance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in cad6b69, passed through env as ONLY. Also DRY_RUN, which as a boolean arrives as the string "false" and would have passed --dry-run when switched off.

async function findConsumers(api, only) {
const repos = [];
for (let page = 1; ; page += 1) {
if (only) {

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: An excluded only target reports 0 consumers and exits 0, which reads like "in sync". This happens when the named repo is archived, a fork, or lacks the marker. Please log why the named repo was excluded, or treat it as an error.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in cad6b69. A named repo that is archived, a fork, or without the marker now says which, and exits 1. Live: --only=kolibri-app gives "kolibri-app is not a consumer: archived".

@@ -0,0 +1,44 @@
# Testing in a separate organization

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: This page is beyond #98's deliverables, which list the sandbox org as out of scope. #98 asks only for docs/testing-automations.md plus a link from automation.md. A full setup guide adds review and maintenance surface. Please confirm scope with the issue author, or move this page to a follow-up PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The page was requested by the issue author during implementation, so #98 is now updated to cover it rather than the page being dropped.

Without `only`, the workflow considers every consumer in the organization, which is how it runs on
its schedule.

To see it propose a change, modify `.github/workflows/automation.yml` in `test-actions` so that it

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: Closing the test sync PR without reverting the drift leaves test-actions out of sync. The weekly run then keeps reopening that PR. Please change "Clearing up" to say: merge the sync PR, or revert the drift.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed in cad6b69 to say merge or revert the drift.

One correction: the pull request is not reopened. Closing it unmerged gives declined, and the workflow deliberately stops proposing until the template changes. The real cost is quieter, that test-actions sits drifted and reports declined rather than in-sync.

- `HOLIDAY_MESSAGE`: Message text

Before/after holidays, toggle `enabled:` for `holiday-message` in [`automation-registry.yml`](../automation-registry.yml) and regenerate (see [`docs/automation.md`](./automation.md)) - no per-repo changes needed.
Before/after holidays, toggle `enabled:` for `holiday-message` in [`automation-registry.yml`](../automation-registry.yml) and regenerate (see [the automation entry point](./automation.md)) - no per-repo changes needed.

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

nitpick: This link-text rewording is unrelated to #98. Please drop it from this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated so both internal doc links read the same way.

assert.equal(reset.body.force, true);
});

test('only limits the run to that repo, and never lists the org', async () => {

@rtibblesbot rtibblesbot Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

praise: Asserting no call touches /orgs/ pins the actual containment guarantee, not just the result count.

Only --only=repo was parsed, so --only repo fell through to a full run against
every consumer, opening real pull requests. That is the spelling #98 uses, so
following the issue produced the opposite of what it asked for. Both forms now
work, and an --only with no repo name is an error rather than a full run.

A named repo that is archived, a fork, or without the marker reported no
consumers and exited 0, which reads as in sync. It now says which of those it
was.

Errors reaching the top print their message and exit 1, rather than a stack
trace.

The workflow passes the input through env instead of expanding it into the
shell of a step holding an org-wide write token.

Clearing up now says to merge the sync pull request or revert the drift.
Closing it unmerged leaves the repo out of sync and reported as declined.

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR #99: all 6 prior findings resolved or acknowledged. No new findings. CI passing.

Prior-finding status

RESOLVED — scripts/sync-automation-template.js — --only <repo> form silently ignored
RESOLVED — .github/workflows/sync-automation-template.yml — inputs.only expanded directly into shell
RESOLVED — scripts/sync-automation-template.js:142 — excluded only target reports 0 consumers and exits 0
ACKNOWLEDGED — docs/testing-in-a-separate-org.md:1 — page beyond #98's scope
RESOLVED — docs/testing-automations.md:44 — closing test sync PR leaves test-actions drifted
ACKNOWLEDGED — docs/community-automations.md:76 — unrelated link-text rewording


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:

  • Retrieved prior bot reviews via the GitHub API
  • Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
  • Only raised NEW findings for newly introduced code
  • Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
  • Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence

The surrounding steps are instructions, so the fork step reads as one too.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Documentation makes sense, and code changes seem right.

@rtibbles
rtibbles merged commit cf38047 into main Sep 25, 2026
2 checks passed
@rtibbles
rtibbles deleted the test-actions-testing-flow branch September 25, 2026 16:36
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.

Use test-actions as the standard place to test the automations

3 participants