chore: Add AI agent roles, skills inventory, and Copilot instructions for Playwright framework#62
chore: Add AI agent roles, skills inventory, and Copilot instructions for Playwright framework#62
Conversation
…EADME with CLI guide and MCP section Co-authored-by: charlyautomatiza <89928062+charlyautomatiza@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds repository-specific guidance to standardize AI-assisted development and Playwright testing practices, aligning MCP-enabled workflows and GitHub Copilot suggestions with the project’s Playwright config, POM structure, and CI setup.
Changes:
- Add
AGENTS.mddefining MCP-oriented agent roles (CLI Automator, Locator Optimizer, CI/CD Engineer). - Add
SKILLS.mddocumenting established Playwright/TypeScript patterns, tooling, reporting, network mocking, and dependencies. - Add
.github/copilot-instructions.mdplus README sections for CLI usage and MCP integration cross-links.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
SKILLS.md |
New “skills inventory” documentation for CLI, assertions, network patterns, POM/fixtures, env vars, a11y, dependencies |
AGENTS.md |
New agent-role definitions with project-specific commands and locator strategy guidance |
.github/copilot-instructions.md |
New Copilot rules for locators, test structure, assertions, reporting, and security |
README.md |
Adds Playwright CLI guide + MCP Integration section linking to the new docs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SKILLS.md
Outdated
|
|
||
| - `page.waitForTimeout()` – flaky; use web-first assertions instead. | ||
| - `page.waitForSelector()` – replaced by locator auto-waiting. | ||
| - `element.innerText()` for assertions – use `expect(locator).toHaveText()` instead. |
There was a problem hiding this comment.
This section says element.innerText() should be avoided for assertions, but the current Tasks.getTaskTitle() implementation returns innerText() and is used in assertions (e.g., tests/api.ui.spec.ts). Since this file is positioned as an inventory of established patterns, consider rephrasing to “prefer”/“migrate towards” web-first locator assertions, or align the codebase first and then document it as established.
| - `element.innerText()` for assertions – use `expect(locator).toHaveText()` instead. | |
| - Prefer `expect(locator).toHaveText()` over using `element.innerText()` for assertions; legacy `innerText()` usages should be migrated over time. |
There was a problem hiding this comment.
Fixed in a4c3ba3. The innerText() guidance has been moved to the agnostic .agents/skills/web-first-assertions.md with the accurate phrasing: "Avoid extracting values with locator.innerText() / locator.textContent() and then asserting on them; this creates a race condition. Use expect(locator).toHaveText() instead." — reflecting it as a migration target rather than a current project standard.
AGENTS.md
Outdated
| 4. `getByText` / `getByTestId` – for content or custom `data-testid` attributes | ||
| 5. `locator('css')` – only when no semantic alternative exists |
There was a problem hiding this comment.
The locator priority list here groups getByText / getByTestId together, but .github/copilot-instructions.md later defines them as separate priorities (text before test id). To avoid conflicting guidance, align the ordering between the docs (either split these into distinct priorities here or adjust the other doc).
| 4. `getByText` / `getByTestId` – for content or custom `data-testid` attributes | |
| 5. `locator('css')` – only when no semantic alternative exists | |
| 4. `getByText` – for visible user-facing text content | |
| 5. `getByTestId` – for custom `data-testid` attributes when semantic/text options are not suitable | |
| 6. `locator('css')` – only when no semantic alternative exists |
There was a problem hiding this comment.
Aligned in a4c3ba3. AGENTS.md now lists getByText and getByTestId as separate priorities (4 and 5), matching the order in copilot-instructions.md.
.github/copilot-instructions.md
Outdated
|
|
||
| ## 🔍 Modern Locators (mandatory) | ||
|
|
||
| - **Prohibited:** `page.$()`, `page.$$()`, `page.$eval()`, `page.$$eval()`. These are deprecated and must not be used. |
There was a problem hiding this comment.
Playwright’s page.$() / $eval() APIs are still supported; calling them “deprecated” is inaccurate and may confuse readers. If the intent is to ban them in this repo, consider rewording to “disallowed in this repository (prefer Locator API for stability)” without claiming deprecation.
| - **Prohibited:** `page.$()`, `page.$$()`, `page.$eval()`, `page.$$eval()`. These are deprecated and must not be used. | |
| - **Prohibited:** `page.$()`, `page.$$()`, `page.$eval()`, `page.$$eval()`. These are disallowed in this repository; prefer the Locator API for stability and consistency. |
There was a problem hiding this comment.
Fixed in a4c3ba3. Changed to: "These are disallowed in this repository; prefer the Locator API for stability and consistency."
.github/copilot-instructions.md
Outdated
| - **Fixtures pattern:** Do NOT instantiate `new Page()` or call `browser.newPage()` inside tests. Always receive `page` through the test fixture parameter: | ||
| ```typescript | ||
| // ❌ New Page Pattern – avoid | ||
| test('my test', async ({ browser }) => { | ||
| const page = await browser.newPage(); | ||
| // ... | ||
| }); | ||
|
|
||
| // ✅ Fixture pattern – always use this | ||
| test('my test', async ({ page }) => { | ||
| const login = new Login(page); | ||
| // ... | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
This rule forbids browser.newPage() inside tests, but tests/a11y.lighthouse.spec.ts currently launches Chromium manually and uses browser.newPage() (required for Lighthouse flow). To keep repo guidance internally consistent, scope this rule to UI tests using @playwright/test fixtures (or explicitly document the a11y/Lighthouse exception).
| - **Fixtures pattern:** Do NOT instantiate `new Page()` or call `browser.newPage()` inside tests. Always receive `page` through the test fixture parameter: | |
| ```typescript | |
| // ❌ New Page Pattern – avoid | |
| test('my test', async ({ browser }) => { | |
| const page = await browser.newPage(); | |
| // ... | |
| }); | |
| // ✅ Fixture pattern – always use this | |
| test('my test', async ({ page }) => { | |
| const login = new Login(page); | |
| // ... | |
| }); | |
| ``` | |
| - **Fixtures pattern (UI tests):** For UI tests using `@playwright/test` fixtures, do NOT instantiate `new Page()` or call `browser.newPage()` inside tests. Always receive `page` through the test fixture parameter: | |
| ```typescript | |
| // ❌ New Page Pattern – avoid (UI tests) | |
| test('my test', async ({ browser }) => { | |
| const page = await browser.newPage(); | |
| // ... | |
| }); | |
| // ✅ Fixture pattern – always use this in UI tests | |
| test('my test', async ({ page }) => { | |
| const login = new Login(page); | |
| // ... | |
| }); |
Note:
tests/a11y.lighthouse.spec.tsis an intentional exception. Lighthouse flows manually launch Chromium and usebrowser.newPage(); this is allowed there but should not be copied into regular UI tests.
There was a problem hiding this comment.
Fixed in a4c3ba3. The rule is now scoped to "UI tests using @playwright/test fixtures" with an explicit callout: tests/a11y.lighthouse.spec.ts intentionally uses browser.newPage() for the Lighthouse remote-debugging port — this is allowed there and must not be copied into regular UI tests.
README.md
Outdated
| npx playwright codegen $BASE_URL | ||
| # Use the baseURL configured in playwright.config.ts | ||
| # e.g. https://charlyautomatiza.github.io/task-management-frontend | ||
|
|
||
| Opens a browser with a recorder panel. Every click, fill, and navigation is transcribed into TypeScript code that you can paste directly into a spec file. | ||
|
|
There was a problem hiding this comment.
npx playwright codegen $BASE_URL assumes a BASE_URL env var, but the repo doesn’t define it (and Playwright’s config use.baseURL is set directly). Consider either using the literal URL from playwright.config.ts, or documenting how to set BASE_URL (e.g., via .env) so this command works as written.
| npx playwright codegen $BASE_URL | |
| # Use the baseURL configured in playwright.config.ts | |
| # e.g. https://charlyautomatiza.github.io/task-management-frontend | |
| Opens a browser with a recorder panel. Every click, fill, and navigation is transcribed into TypeScript code that you can paste directly into a spec file. | |
| npx playwright codegen https://charlyautomatiza.github.io/task-management-frontend | |
| # Replace the URL above with the baseURL configured in your playwright.config.ts (use.baseURL) | |
| Opens a browser with a recorder panel. Every click, fill, and navigation is transcribed into TypeScript code that you can paste directly into a spec file. |
There was a problem hiding this comment.
Fixed in a4c3ba3. Replaced $BASE_URL with the actual URL from playwright.config.ts and added a comment explaining where to find the value: # Replace the URL above with the baseURL configured in your playwright.config.ts (use.baseURL).
SKILLS.md
Outdated
| Direct API calls are made using `playwright.request.newContext()`. The `baseURL` for the API is set per-context (see `tests/global.setup.ts` and `tests/api.ui.spec.ts`; the URL is stored as `process.env.API_BASE_URL` or hard-coded in those files and must be moved to `.env` for different environments). The pattern used in this project: | ||
|
|
||
| ```typescript | ||
| let apiContext: APIRequestContext; | ||
|
|
||
| test.beforeEach(async ({ playwright }) => { | ||
| apiContext = await playwright.request.newContext({ | ||
| baseURL: 'https://task-mgmt-charlyautomatiza.onrender.com', | ||
| extraHTTPHeaders: { Accept: 'application/json' }, |
There was a problem hiding this comment.
process.env.API_BASE_URL is referenced here as an existing pattern, but it doesn’t appear anywhere else in the repo (all current API contexts hard-code https://task-mgmt-charlyautomatiza.onrender.com). Either update this doc to reflect the current state, or actually introduce and document an API_BASE_URL env var (and use it in tests/global.setup.ts / API specs).
There was a problem hiding this comment.
Fixed in a4c3ba3. The project-specific APIRequestContext documentation (which referenced the non-existent process.env.API_BASE_URL) has been removed from the root. The generalist .agents/skills/network-interception.md now shows the correct pattern using process.env.API_BASE_URL ?? 'https://your-api.example.com' as a documented best-practice placeholder.
|
.agents/skills/ es la ruta correcta para las skills @copilot por otra parte quiero que la skill sea agnóstica, sea generalista y no atada al contexto específico de este proyecto. Tiene que seguir la estructura y patrón estándar de Anthropic |
…ors, assertions and codegen URL Co-authored-by: charlyautomatiza <89928062+charlyautomatiza@users.noreply.github.com>
Done in a4c3ba3. Removed
Each skill is self-contained, generalist, and carries no project-specific context. |
Standardises the repo for AI-assisted development by adding structured guidance for MCP-enabled editors and GitHub Copilot, grounded in the actual project structure (
playwright.config.ts,tests/pageobjects/, CI workflow).New files
AGENTS.md— Defines three MCP agent roles with concrete CLI commands and project-specific context:--debug,--ui,codegen,show-trace,--last-failedlogin.ts/tasks.tsto theirgetByRole/getByLabel/getByPlaceholderequivalents; locator priority aligned across all docs (getByRole→getByLabel→getByPlaceholder→getByText→getByTestId→ CSS).agents/skills/.agents/skills/— Three project-agnostic, generalist skill files following Anthropic's standard structure (replacing the rootSKILLS.md):playwright-cli-tooling.md— running, debugging, recording, and inspecting tests via the Playwright CLIweb-first-assertions.md— auto-retryingexpectassertions for DOM, URL, and API statenetwork-interception.md— mocking, aborting, and inspecting network requests withpage.route().github/copilot-instructions.md— Copilot golden rules:page.$()and XPath chains disallowed in this repository; enforces 6-level locator prioritybrowser.newPage()) forbidden in UI tests;tests/a11y.lighthouse.spec.tsdocumented as an explicit exception (required for Lighthouse remote-debugging port)expectassertions only; nowaitForTimeout().env+ GitHub Actions secretsUpdated
README.md— Adds a Playwright CLI Guide section (--ui,--debug,codegen,show-trace, sharding) and an MCP Integration section with agent table, local setup steps, example prompts, and cross-links to the new docs.Removed
SKILLS.md(root) — replaced by the agnostic per-skill files under.agents/skills/.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.