Issue 4940: Add skip deployment flag for PR bot tests

[stuart-bass-cgi]

Resolves #4940

What is being addressed

PR bot /test and /test-extended commands currently always run the full validation flow, including build and deployment jobs, even when the PR validation environment has already been deployed. This makes it slow to re-run smoke or extended E2E tests after transient test failures or test-only changes.

How is this addressed

• Adds a skip_deployment flag for the /test and /test-extended PR bot commands.
• Passes the parsed flag from the PR comment bot into the reusable TRE deployment workflow.
• When skip_deployment is true, skips the deployment/build/register jobs and allows the smoke and requested custom E2E jobs to run against the existing deterministic PR validation environment.
• Keeps the existing external PR SHA validation in place, including when skip_deployment is supplied.
• Updates PR bot command documentation with the new syntax and fork PR examples.
• Adds unit test coverage for the new command parsing behaviour.

[Copilot]

Pull request overview

Adds a skip_deployment flag to PR comment bot /test and /test-extended commands so maintainers can rerun smoke/extended E2E tests against an already-deployed deterministic PR validation environment, avoiding the full build/deploy pipeline when appropriate.

Changes:

• Extended PR bot command parsing to recognize skip_deployment and pass it through to the reusable deployment workflow.
• Updated the reusable workflow to conditionally bypass deployment/build stages and still run smoke/custom E2E jobs.
• Updated developer docs and added unit tests covering the new command parsing behavior.

Reviewed changes

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

Show a summary per file

File	Description
docs/tre-developers/github-pr-bot-commands.md	Documents the new skip_deployment flag syntax and fork PR examples for /test and /test-extended.
.github/workflows/pr_comment_bot.yml	Plumbs skipDeployment output from the parsing script into the reusable workflow call.
.github/workflows/deploy_tre_reusable.yml	Adds skipDeployment input and adjusts E2E job conditions to allow running tests when deployment is skipped.
.github/scripts/build.js	Parses skip_deployment, emits skipDeployment output, and updates external-PR SHA extraction to ignore the flag.
.github/scripts/build.test.js	Adds unit tests validating skipDeployment output behavior for internal and external PR command forms.

[stuart-bass-cgi]

@microsoft-github-policy-service agree company="CGI"

[marrobi]

@stuart-bass-cgi thanks for the PR.

A review with Opus 4.8 says:

I

---

Overall: Well-structured, focused change that solves a real developer pain point (re-running smoke/extended E2E tests without a full redeploy). Good test coverage and docs, plus a couple of nice opportunistic bug fixes. A few items to confirm before approval.

🟢 Done well

• Clean separation of commandHasSkipDeploymentFlag() and getShaFromCommandParts() — correctly supports passing a SHA and the flag in any order on fork PRs.
• Good unit-test coverage for internal true/default-false, fork /test <sha> skip_deployment, and /test-extended.
• Fixes real pre-existing bugs: branchRefid → branchRefId typo, and the debug echo that printed prRefId under the branchRefId label.
• Docs updated in step with the code, including fork examples.

🟠 To address

1. Check status reporting on skip-deployment runs. With skipDeployment == true, deploy_management is skipped, so the entire needs chain feeding e2e_tests_smoke/e2e_tests_custom is skipped. The always() && (inputs.skipDeployment || ...) guards make the tests run, which works — but please confirm the summary job still reports the "Deploy PR / Run E2E Tests (Smoke)" check back to the PR head SHA so the PR status check updates correctly. A short code comment explaining why always() + skipped upstreams produces the intended behavior would help future maintainers.
2. skip_deployment is silently ignored by /test-extended-aad and /test-shared-services. The flag is only wired into /test and /test-extended. A user appending it to the other commands gets a full deploy with no feedback. Consider warning on unsupported use, or confirm this asymmetry is intentional (docs already omit it for those commands).

🟡 Nits

• skip_deployment is a magic string in multiple places — consider a shared constant to avoid drift.
• const { create } = require('domain'); (build.js) is unused dead code; low-risk cleanup while you're here.
• The string→bool round-trip (== 'true') for the typed workflow input is correct — just flagging it's intentional.

Verdict: LGTM pending confirmation of (1) smoke-test check reporting and (2) the intentional flag asymmetry.

---

Let me know if you agree!

[github-actions]

Unit Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit de94daa.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

.github/workflows/deploy_tre_reusable.yml:887

• Same as the smoke job: when deployment jobs are skipped because of an earlier failure, this condition can still evaluate true (since needs.*.result may be skipped rather than failure). That risks running long custom E2E tests against an environment that never successfully deployed.

  e2e_tests_custom:
    name: "Run E2E Tests"
    if: ${{ always() && inputs.e2eTestsCustomSelector != '' && (inputs.skipDeployment || (!contains(needs.*.result, 'failure') && !contains(needs.*.result, 'cancelled'))) }}
    runs-on: ubuntu-latest

.github/workflows/deploy_tre_reusable.yml:842

• The current job condition will allow smoke tests to run when upstream deployment jobs were skipped due to a failure (e.g., a deploy job fails, downstream needs become skipped, and this expression doesn’t catch that). This can run E2E tests against a partially/un-deployed environment.

This issue also appears on line 884 of the same file.

  e2e_tests_smoke:
    name: "Run E2E Tests (Smoke)"
    if: ${{ always() && (inputs.skipDeployment || (!contains(needs.*.result, 'failure') && !contains(needs.*.result, 'cancelled'))) }}
    runs-on: ubuntu-latest

.github/scripts/build.test.js:416

• Unit tests cover skip_deployment for maintainer PRs and for /test on external PRs, but there’s no coverage for /test-extended <sha> skip_deployment on external PRs. Since the SHA parsing changed to ignore the skip_deployment token, adding a test for the external /test-extended variant would protect against regressions (including different token ordering).

      describe(`for '/test-extended'`, () => {
        test(`should set command to 'run-tests-extended'`, async () => {
          const context = createCommentContext({
            username: 'admin',
            body: '/test-extended',
          });
          await getCommandFromComment({ core, context, github });
          expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-extended');
        });

        test(`should set skipDeployment to 'true' when skip_deployment is supplied`, async () => {
          const context = createCommentContext({
            username: 'admin',
            body: '/test-extended skip_deployment',
          });
          await getCommandFromComment({ core, context, github });
          expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-extended');
          expect(outputFor(mockCoreSetOutput, 'skipDeployment')).toBe('true');
        });