Skip to content

feat(tests): manually run functional tests on pr#20580

Open
nshirley wants to merge 1 commit into
mainfrom
worktree-gated-functional-test-pr
Open

feat(tests): manually run functional tests on pr#20580
nshirley wants to merge 1 commit into
mainfrom
worktree-gated-functional-test-pr

Conversation

@nshirley
Copy link
Copy Markdown
Contributor

@nshirley nshirley commented May 12, 2026

Because

  • We want to reduce the ci credit usage from running functional tests on every pr push

This pull request

  • Gates PR functional tests behind an approval step
  • Adds a script to trigger tests locally for a given branch
  • Adds a Claude skill to help run the tests as well

Issue that this pull request solves

Closes: FXA-13697

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

This makes it so the Functional Tests for PR are only run with an approval. As such, there's now a script which you can run manually, or a Claude skill which calls the script. The nice thing about the skill is that we can use it with loop skill to have Claude check if approval is ready and then wait and try again!

image

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

Copilot AI review requested due to automatic review settings May 12, 2026 16:35
@nshirley nshirley requested a review from a team as a code owner May 12, 2026 16:35
Copy link
Copy Markdown
Contributor

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

This PR reduces CircleCI usage by gating PR functional (Playwright) tests behind a pipeline parameter and providing manual triggers (local script + Claude skill) to run them on demand.

Changes:

  • Add enable_functional_tests pipeline parameter and move PR functional tests into a separate conditional workflow.
  • Add _scripts/trigger-functional-tests.sh to trigger the manual workflow and cancel prior runs on the same branch.
  • Add a Claude skill (/run-functional-tests) that guides users through invoking the trigger script.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
.claude/skills/run-functional-tests/SKILL.md Adds a user-invocable Claude skill documenting how to trigger manual PR functional tests.
.circleci/config.yml Introduces a new pipeline parameter and conditional workflow for manual functional test runs; removes functional tests from default PR workflow.
_scripts/trigger-functional-tests.sh Adds a branch-aware CircleCI trigger script that starts a functional-only pipeline and cancels prior functional workflows on the same branch.

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

Comment thread _scripts/trigger-functional-tests.sh Outdated
Comment thread .claude/skills/run-functional-tests/SKILL.md Outdated
Comment thread .claude/skills/run-functional-tests/SKILL.md Outdated
@nshirley nshirley force-pushed the worktree-gated-functional-test-pr branch from 4b338dc to cdb68d9 Compare May 12, 2026 18:04
Comment thread .circleci/config.yml Outdated
@nshirley nshirley force-pushed the worktree-gated-functional-test-pr branch 3 times, most recently from af7947a to 23528d7 Compare May 12, 2026 20:22
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@nshirley what are your thoughts on running the functional tests via branch name (ie anything containing ci), or even commit message containing ([run-ci]). That way we don't need a skill or bash script.

Kinda leaning towards commit message since that gives good flexibility.

@nshirley
Copy link
Copy Markdown
Contributor Author

@vbudhram I suppose that could work too. I'd be worried we would forget to add the CI in the branch name, and the commit message means we'd have to commit and push a unique message to run tests, then rebase after tests pass and push again, then wait for build/unit/integration again. We still want to 'require' functional tests before merging, just don't want to run them on every single push

This approach just adds an approve button to the circle workflow and the script/skill are just convenience options to trigger the approval without having to go open up Circle.

Here's one that I approved, but that page just shows a button to approve it if it's pending.
image

Comment thread .circleci/config.yml Outdated
@nshirley nshirley force-pushed the worktree-gated-functional-test-pr branch from 23528d7 to cd2ac5c Compare May 13, 2026 19:25
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@nshirley Lets give this a try, just gotta make sure we let folks know they have to manually opt into running the tests.

Comment thread _scripts/approve-functional-tests.sh Outdated
# a friendly message on HTTP errors so callers don't have to interpret raw
# error responses (e.g. an HTML 401 body that would otherwise blow up jq).
# Usage: circleci_api METHOD URL [extra-curl-args...]
circleci_api() {
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.

This is gross. Let's have the user install circleci cli tool. They should have it anyways as there other benefits especially when working with Claude.

Comment thread _scripts/approve-functional-tests.sh Outdated

set -euo pipefail

for cmd in curl jq git; do
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.

We could require the circleci command line utility instead, which I think would be better. Since these command line tools are so helpful, I think we should make them required as part of the development setup. I'll update our internal docs accordingly.

Copy link
Copy Markdown
Contributor

@dschom dschom left a comment

Choose a reason for hiding this comment

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

Would it be a big deal to update the script to use the circleci cli tool. I think this will be result in a shorter more robust script.

@nshirley
Copy link
Copy Markdown
Contributor Author

@dschom no issue updating this! I have the cli installed too but API felt more flexible, but I'll re-work it to clean it up!

Because:
- We want to reduce the ci credit usage from running functional tests on every pr push

This pull request:
- Gates PR functional tests behind a ci parameter
- Adds a script to trigger tests locally for a given branch
- Adds a Claude skill to help run the tests as well

Closes: FXA-13697
@nshirley nshirley force-pushed the worktree-gated-functional-test-pr branch from cd2ac5c to 2eef1af Compare May 15, 2026 16:57
@nshirley
Copy link
Copy Markdown
Contributor Author

@dschom, following up here from our thread. The CLI does not provide the ability to approve pending workflows and it appears the majority of functionality is supposed to go through the API. To that, I did my best to clean the script up and make it a bit nicer - it was definitely over engineered

@nshirley nshirley requested a review from dschom May 15, 2026 22:20
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.

4 participants