-
Notifications
You must be signed in to change notification settings - Fork 152
Add PR integration test skill #482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| --- | ||
| name: pr-integration-test | ||
| description: 'Design, implement, and validate Intelligent Terminal integration tests for a target pull request or regression. Use when asked to add PR integration tests, convert a bug fix into E2E coverage, prove existing behavior still works, map tests to the release checklist, or verify Invoke-ItE2EReport marks checklist cases complete.' | ||
| --- | ||
|
|
||
| # PR Integration Test | ||
|
|
||
| Turn a target PR into durable, behavior-focused integration coverage that proves | ||
| the fixed path, protects existing behavior, and updates the generated release | ||
| checklist. | ||
|
|
||
| ## When to Use This Skill | ||
|
|
||
| - Add integration or E2E tests for a specific PR. | ||
| - Add follow-up regression coverage after a fix has merged. | ||
| - Prove a bug from an issue cannot recur across real component boundaries. | ||
| - Extend `doc/release-check-list.md` and make report scripts check the new rows. | ||
| - Audit whether a PR's existing tests cover the user-visible behavior rather | ||
| than only its implementation details. | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - Work on a feature branch, never directly on `main` or `master`. | ||
| - Use `gh` to inspect the target PR, linked issues, review discussion, and state. | ||
| - Read repository and area-specific instructions before editing. | ||
| - Read `test/e2e/README.md` and reuse the ItE2E framework instead of creating a | ||
| parallel harness. | ||
| - Run `pwsh -File test/e2e/bootstrap.ps1 -Check` before live E2E validation. | ||
|
|
||
| ## Workflow | ||
|
|
||
| Follow [workflow.md](./references/workflow.md). Track the phases with a TODO | ||
| list because PR analysis, test design, checklist wiring, live validation, and | ||
| delivery must all complete. | ||
|
|
||
| 1. Resolve the target PR and select the correct base commit and branch strategy. | ||
| 2. Reconstruct the user-visible behavior and the complete component path. | ||
| 3. Audit existing unit, integration, E2E, and release-checklist coverage. | ||
| 4. Build a positive/negative/regression behavior matrix before writing tests. | ||
| 5. Implement the smallest deterministic integration suite using existing ItE2E | ||
| primitives. | ||
| 6. Map each new release-signoff behavior to a stable checklist item. | ||
| 7. Run the new suite, related existing regressions, and release-report scripts. | ||
| 8. Commit, push, and prepare a PR whose evidence names passes, skips, checklist | ||
| IDs, and the exact build tested. | ||
|
|
||
| ## Test Design Standard | ||
|
|
||
| For every proposed case, record: | ||
|
|
||
| | Field | Required answer | | ||
| |-------|-----------------| | ||
| | Contract | What user-visible behavior must remain true? | | ||
| | Trigger | What exact action or input exercises it? | | ||
| | Boundary | Which real component handoff does this test add beyond unit tests? | | ||
| | Oracle | What deterministic observable proves success or suppression? | | ||
| | Negative control | What similar input must not trigger the behavior? | | ||
| | Existing protection | Which existing tests protect old behavior and must still run? | | ||
| | Checklist title | Which exact bold release-checklist title contains the Pester test name? | | ||
|
|
||
| An integration test is justified only when it proves a boundary that lower-level | ||
| tests do not. Keep focused unit tests for branch logic; use E2E for wiring, | ||
| packaging, process boundaries, protocol events, persistence, and real UI | ||
| interaction. | ||
|
|
||
| ## Oracle Priority | ||
|
|
||
| Prefer the earliest deterministic product-owned signal: | ||
|
|
||
| 1. Protocol/event stream | ||
| 2. Persisted or queryable application state | ||
| 3. Structured diagnostic log | ||
| 4. Rendered terminal or UI state | ||
| 5. LLM-generated text | ||
|
|
||
| Use an LLM output or AI judge only when usefulness or semantic correctness is | ||
| the behavior under test. Do not make routing, trigger, suppression, or | ||
| single-flight regressions depend on model wording. | ||
|
|
||
| ## Release Checklist Contract | ||
|
|
||
| - Add one unchecked `[E2E]` checklist item per independently releasable behavior. | ||
| - Give each item a concise bold title that appears verbatim in the matching | ||
| Pester full name (`Describe.Context.It`). | ||
| - Prefer exact-title matching. Add `test/e2e/release-coverage-map.psd1` entries | ||
| only when an exact test name would be misleading or one case intentionally | ||
| covers multiple checklist items. | ||
| - Run `pwsh -File test/e2e/Set-ChecklistIds.ps1`; never assign or renumber | ||
| `Cnnn` IDs manually. | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| - Run the suite through `Invoke-ItE2EReport.ps1`, not only `Invoke-Pester`, and | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| inspect `release-report.md` to prove every new ID is `[x]`. | ||
| - Verify `Update-ReleaseReport.ps1` too when the suite is expected to support | ||
| partial release-signoff runs. | ||
|
|
||
| ## Completion Gate | ||
|
|
||
| Do not call the work complete until all of these are true: | ||
|
|
||
| - The original regression fails on the old behavior or is otherwise tied to a | ||
| verified pre-fix symptom. | ||
| - The fixed path passes through the real integration boundary. | ||
| - False-positive and replay/idempotency risks are covered where applicable. | ||
| - Related pre-existing suites pass, or every skip/failure is explicitly | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| classified as environment, model variance, product regression, or test bug. | ||
| - New checklist IDs become `[x]` through both applicable report paths. | ||
| - The deployed package or executable under test is proven to contain the target | ||
| change; no stale artifact is being exercised. | ||
|
|
||
| ## Gotchas | ||
|
|
||
| - **Do not test only the implementation detail named in the PR.** Reconstruct | ||
| the end-to-end user path and assert the observable contract. | ||
| - **Do not duplicate a unit test at E2E level.** Add the missing process, | ||
| protocol, persistence, packaging, or UI boundary. | ||
| - **Do not use a successful lower-layer event as proof of the final feature.** | ||
| When the contract is downstream, assert both the trigger and its downstream | ||
| effect. | ||
| - **Do not turn product failures into skips.** Skip only when an external | ||
| prerequisite is genuinely unavailable. A connected product that behaves | ||
| incorrectly must fail. | ||
| - **Do not rely on fixed sleeps for positive completion.** Start listeners | ||
| before actions and poll for a scoped event/state. Use a short bounded | ||
| observation window only to prove that something does not happen. | ||
| - **Do not let unrelated panes or windows satisfy assertions.** Scope events by | ||
| pane/session/tab/window identifiers whenever the protocol exposes them. | ||
| - **Do not trust a build command alone.** Confirm package selection, deployed | ||
| version, co-located binaries, and runtime logs when stale artifacts are | ||
| possible. | ||
| - **Do not add checklist text without validating report matching.** A passing | ||
| Pester case that leaves its release row unchecked is incomplete coverage. | ||
|
|
||
| ## References | ||
|
|
||
| - [Detailed PR-to-integration-test workflow](./references/workflow.md) | ||
| - [ItE2E framework](../../../test/e2e/README.md) | ||
| - [Release checklist](../../../doc/release-check-list.md) | ||
259 changes: 259 additions & 0 deletions
259
.github/skills/pr-integration-test/references/workflow.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,259 @@ | ||
| # PR-to-Integration-Test Workflow | ||
|
|
||
| Use this procedure for a target Intelligent Terminal PR. Adapt the exact test | ||
| suite and build commands to the files changed by that PR. | ||
|
|
||
| ## 1. Resolve the Target and Branch Strategy | ||
|
|
||
| Inspect the PR before reading implementation files: | ||
|
|
||
| ```powershell | ||
| gh pr view <number> --repo microsoft/intelligent-terminal ` | ||
| --json number,title,body,state,mergedAt,mergeCommit,baseRefName,headRefName,commits,files,closingIssuesReferences,reviews,url | ||
| gh pr diff <number> --repo microsoft/intelligent-terminal | ||
|
vanzue marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
|
||
| Read linked issues and relevant review threads. Record: | ||
|
|
||
| - The pre-fix user symptom and exact reproduction. | ||
| - The intended behavior and explicitly accepted limitations. | ||
| - The changed components and every boundary crossed at runtime. | ||
| - Whether the PR is open, merged, or superseded. | ||
|
|
||
| Choose the branch deliberately: | ||
|
|
||
| - **Merged PR / follow-up test PR:** update the PR's base branch with | ||
| `git pull --ff-only`, delete the merged feature branch only after verifying | ||
| merge state, then create a new test branch from the merged commit. | ||
| - **Open PR and tests belong in it:** use the PR head only when the user expects | ||
| commits on that branch and it is safe to push there. | ||
| - **Open PR but independent validation is requested:** create a branch from the | ||
| PR head and clearly state that the test branch depends on the unmerged PR. | ||
|
|
||
| Never force-delete a branch based only on its name. Verify the PR state and a | ||
| clean worktree first. A squash-merged branch may require `git branch -D` | ||
| because Git cannot infer ancestry even though GitHub confirms the merge. | ||
|
|
||
| ## 2. Reconstruct the Behavioral Contract | ||
|
|
||
| Describe the full path as components and observable handoffs: | ||
|
|
||
| ```text | ||
| user trigger | ||
| -> producer/component A | ||
| -> protocol or process boundary | ||
| -> consumer/component B | ||
| -> user-visible effect | ||
| ``` | ||
|
|
||
| For a regression, distinguish: | ||
|
|
||
| - What the producer emitted before the fix. | ||
| - What downstream code interpreted. | ||
| - Why existing tests did not catch the gap. | ||
| - Which observable separates the regression from a legitimate success case. | ||
|
|
||
| Do not accept a PR description as the sole source of truth. Compare it with the | ||
| code, issue reproduction, logs/events, and live behavior when available. | ||
|
|
||
| ## 3. Audit Existing Coverage and Harness Primitives | ||
|
|
||
| Search before adding helpers: | ||
|
|
||
| ```powershell | ||
| git grep -n -E "<feature|event|setting|command>" -- test/e2e tools/wta/src src/cascadia | ||
|
vanzue marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
|
||
| Read: | ||
|
|
||
| - Related `test/e2e/tests/Feature.*.Tests.ps1` suites. | ||
| - Relevant functions under `test/e2e/ItE2E/Public/`. | ||
| - Unit tests around classification, state transitions, and parsing. | ||
| - Matching items in `doc/release-check-list.md`. | ||
| - `test/e2e/release-coverage-map.psd1`. | ||
| - `test/e2e/release-exclude.psd1`, so a new title is not silently omitted from | ||
| the generated report. | ||
|
|
||
| Classify current coverage: | ||
|
|
||
| | Layer | What it should prove | | ||
| |-------|----------------------| | ||
| | Unit | Local decisions, parsing, reducer/state-machine branches | | ||
| | Component | Generated scripts, serialization, API adapters | | ||
| | Integration/E2E | Real process/protocol/package/UI wiring | | ||
| | Release checklist | Which user-facing behaviors count as signed off | | ||
|
|
||
| Reuse public ItE2E primitives. Add a shared helper only when multiple suites | ||
| need the same operation or the helper itself provides a more precise oracle. | ||
|
|
||
| ## 4. Build the Behavior Matrix | ||
|
|
||
| Start with the regression, then add only risk-driven controls: | ||
|
|
||
| 1. **Regression positive:** the exact old failure now reaches the intended final | ||
| effect. | ||
| 2. **Ordinary baseline:** the pre-existing successful or failure path still | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| works. | ||
| 3. **False-positive control:** a similar but legitimate case remains ignored. | ||
| 4. **Replay/idempotency:** redraw, retry, duplicate event, or repeated command | ||
| does not double-submit or reuse stale state. | ||
| 5. **Lifecycle/routing:** hidden panes, split panes, moved tabs, reconnects, or | ||
| process restarts only when the PR touches those risks. | ||
| 6. **Compatibility:** alternate shell, agent, policy, or language mode only when | ||
| the changed code is shared with it. | ||
|
|
||
| Avoid combinatorial matrices. Each case must correspond to a plausible failure | ||
| mode introduced or exposed by the target PR. | ||
|
|
||
| For every case, identify both the immediate trigger and the downstream effect. | ||
| For example, proving a failure event exists is insufficient when the user | ||
| contract is that Autofix receives it; assert the event and the Autofix request. | ||
|
|
||
| ## 5. Implement Deterministic ItE2E Tests | ||
|
|
||
| Follow existing suite structure: | ||
|
|
||
| ```powershell | ||
| #Requires -Modules @{ ModuleName='Pester'; ModuleVersion='5.0.0' } | ||
|
|
||
| BeforeDiscovery { | ||
| $package = Get-AppxPackage | Where-Object Name -like '*IntelligentTerminal*' | ||
| $script:Ready = $null -ne $package | ||
| } | ||
|
|
||
| Describe 'Feature: <behavior>' -Tag 'Feature' -Skip:(-not $script:Ready) { | ||
| BeforeAll { | ||
| Import-Module (Join-Path $PSScriptRoot '..\ItE2E\ItE2E.psd1') -Force | ||
| $script:app = Start-Terminal -Package (Get-ItTestPackage) -PassFre $true | ||
| } | ||
| AfterAll { if ($script:app) { Stop-Terminal -App $script:app } } | ||
|
|
||
| It '<exact release-checklist title>' { | ||
| # Start observers before the action, scope the oracle, and assert the | ||
| # real downstream contract. | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Implementation rules: | ||
|
|
||
| - Start event listeners before triggering the behavior. | ||
| - Scope event predicates by stable IDs; do not accept unrelated startup events. | ||
| - Use `Wait-WtEvent`, `Test-Until`, or assertion helpers for positive outcomes. | ||
| - For negative outcomes, first prove the command/action completed, then observe | ||
| a short bounded window and assert the forbidden event/state is absent. | ||
| - Use unique command text when the product intentionally deduplicates repeated | ||
| requests. | ||
| - Use fresh applications or explicit state cleanup when one case can leave an | ||
| agent turn, setting, pane, or listener active. | ||
| - Put cleanup in `finally`/`AfterAll`. | ||
| - Keep model-semantic tests separate from deterministic routing tests. If model | ||
| variance is accepted, skip only the semantic assertion after proving the | ||
| deterministic pipeline succeeded. | ||
|
|
||
| ## 6. Wire the Release Checklist | ||
|
|
||
| Add unchecked E2E items near the related feature section: | ||
|
|
||
| ```markdown | ||
| - [ ] `[new]` `[E2E]` **Exact behavior title:** User-visible contract. _(#issue; E2E: `Feature.Suite`.)_ | ||
| ``` | ||
|
|
||
| Use the same `Exact behavior title` in the Pester `It` name. Then assign IDs: | ||
|
|
||
| ```powershell | ||
| pwsh -NoProfile -File test/e2e/Set-ChecklistIds.ps1 | ||
| ``` | ||
|
|
||
| Never reuse, insert, or renumber IDs manually. If exact-title matching is not | ||
| appropriate, add a narrow regex to `test/e2e/release-coverage-map.psd1` and | ||
| explain why it cannot over-credit another behavior. | ||
|
|
||
| Update the suite table in `test/e2e/README.md` when adding a new feature file. | ||
|
|
||
| ## 7. Validate Tests and Report Mapping | ||
|
|
||
| Verify prerequisites: | ||
|
|
||
| ```powershell | ||
| pwsh -NoProfile -File test/e2e/bootstrap.ps1 -Check | ||
| ``` | ||
|
|
||
| Run the new suite through the report driver: | ||
|
|
||
| ```powershell | ||
| pwsh -NoProfile -File test/e2e/Invoke-ItE2EReport.ps1 ` | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| -Path test/e2e/tests/Feature.<Name>.Tests.ps1 ` | ||
| -UpdateReport | ||
| ``` | ||
|
|
||
| Confirm: | ||
|
|
||
| - No new case failed or skipped unexpectedly. | ||
| - Every new checklist ID appears as `- [x]` in | ||
| `test/e2e/artifacts/release-report.md`. | ||
| - A failure would produce `AUTOMATION FAILED`, not a false pass. | ||
|
|
||
| `-UpdateReport` incrementally overlays matched results when a prior report | ||
| exists and falls back to a fresh report otherwise. To validate the underlying | ||
| incremental script explicitly or use a custom output directory, run: | ||
|
|
||
| ```powershell | ||
| pwsh -NoProfile -File test/e2e/Update-ReleaseReport.ps1 ` | ||
| -Report <existing-release-report.md> ` | ||
| -ResultsXml <results.xml> ` | ||
| -OutFile <updated-release-report.md> | ||
| ``` | ||
|
|
||
| Verify only matched items changed and every new ID is `[x]`. A skipped-only | ||
| result must leave an existing checkbox unchanged. | ||
|
|
||
| Run related existing suites in the same Pester invocation when practical. At | ||
| minimum include: | ||
|
|
||
| - The suite that previously covered the ordinary path. | ||
| - The lower-layer suite that produces the new trigger. | ||
| - Routing/lifecycle suites affected by shared state. | ||
|
|
||
| Escalate to the broader Feature suite only when targeted runs expose shared | ||
| regressions or the PR changes common harness/product infrastructure. | ||
|
|
||
| ## 8. Prove the Correct Build Ran | ||
|
|
||
| Build and deploy according to the changed area. For WTA + Terminal changes: | ||
|
|
||
| 1. Build WTA with the explicit target matching the package architecture: | ||
| `--target x86_64-pc-windows-msvc` for x64 or | ||
| `--target aarch64-pc-windows-msvc` for ARM64. Package deployment prefers the | ||
| explicit-target artifact. | ||
| 2. Build the C++ package after WTA. | ||
| 3. Deploy/redeploy the package and select it with `ITE2E_PACKAGE` or | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| `-Package Dev`. | ||
| 4. Verify package version/path, runtime logs, or a changed observable. | ||
|
|
||
| Do not infer deployment success from compilation alone. A stale packaged | ||
| `wta.exe`, generated shell-integration script, profile reference, or AppX | ||
| staging directory can make a new test exercise old code. | ||
|
|
||
| ## 9. Deliver the Test PR | ||
|
|
||
| Before committing: | ||
|
|
||
| ```powershell | ||
| git -c core.whitespace=cr-at-eol diff --check | ||
| git status --short --branch | ||
| ``` | ||
|
|
||
| The PR description must include: | ||
|
|
||
| - Target PR and issue. | ||
| - Missing integration boundary now covered. | ||
| - Positive, negative, and compatibility cases added. | ||
| - New checklist IDs and proof they become `[x]`. | ||
| - Exact targeted and regression test totals. | ||
| - Every skip with its reason. | ||
| - Package/build tested. | ||
|
|
||
| Do not claim complete regression coverage if environment-dependent suites were | ||
| not run. Distinguish deterministic passes from accepted model variance and | ||
| unavailable prerequisites. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.