Update coding guidelines in AGENTS.md by compulim · Pull Request #5766 · microsoft/BotFramework-WebChat
Added guidelines for test cases, code coverage, and deprecation notes.
Changelog Entry
Description
More guidelines.
Design
Specific Changes
-
I have added tests and executed them locally -
I have updatedCHANGELOG.md - I have updated documentation
Review Checklist
This section is for contributors to review your work.
-
Accessibility reviewed (tab order, content readability, alt text, color contrast) -
Browser and platform compatibilities reviewed -
CSS styles reviewed (minimal rules, noz-index) - Documents reviewed (docs, samples, live demo)
-
Internationalization reviewed (strings, unit formatting) -
package.jsonandpackage-lock.jsonreviewed -
Security reviewed (no data URIs, check for nonce leak) -
Tests reviewed (coverage, legitimacy)
Added guidelines for test cases, code coverage, and deprecation notes.
compulim
marked this pull request as ready for review
Add guidelines for handling snapshots in tests.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates AGENTS.md to add/expand contributor-facing coding guidelines around repository setup, testing expectations (including coverage), React component conventions, and snapshot update workflow.
Changes:
- Add a pointer to the contributing/setup documentation.
- Add guidelines for tests, code coverage targets, and deprecation dating.
- Restructure the React section and add snapshot-related testing guidance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### General | ||
|
|
||
| - Read `docs/CONTRIBUTING.md` for details on setting up the repository for development |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/CONTRIBUTING.md does not exist in this repo (the contributing guide is under .github/CONTRIBUTING.md). Update this link/path so new contributors are not sent to a missing document.
| - Read `docs/CONTRIBUTING.md` for details on setting up the repository for development | |
| - Read `.github/CONTRIBUTING.md` for details on setting up the repository for development |
Copilot uses AI. Check for mistakes.
| - The only exception is `id`, e.g. `getId()` over `getID()` | ||
| - Use fewer shorthands, only allow `min`, `max`, `num` | ||
| - All new/changed production code must have test cases, look at `__tests__/html2/**` | ||
| - Code coverage for new/changed code should reach 80% |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new “80% code coverage for new/changed code” guideline is hard to apply as written: the repo currently collects coverage but does not define a coverageThreshold, and CI prints overall lcov summaries rather than per-diff coverage. Consider either (1) documenting exactly how to measure this in CI for a PR, or (2) changing the guideline to an enforceable/measurable target (e.g., overall project thresholds or specific packages).
| - Code coverage for new/changed code should reach 80% | |
| - Code coverage is measured using the lcov summary printed in CI for `npm test -- --coverage` | |
| - For each PR, overall project line coverage reported in that summary must be at least 80% | |
| - Do not merge PRs that reduce overall line coverage, unless explicitly approved by a maintainer and documented in the PR description | |
| - When adding substantial new/changed production code, aim for at least 80% line coverage for the affected files (you can verify this locally with `npm test -- --coverage` and inspecting the per-file report) |
Copilot uses AI. Check for mistakes.
| - Use fewer shorthands, only allow `min`, `max`, `num` | ||
| - All new/changed production code must have test cases, look at `__tests__/html2/**` | ||
| - Code coverage for new/changed code should reach 80% | ||
| - Deprecation notes should mark the date as 2 years from now |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“Deprecation notes should mark the date as 2 years from now” is ambiguous and doesn’t match the format used elsewhere (e.g., “will be removed on or after YYYY-MM-DD”). Consider specifying the expected wording/format and what “now” is anchored to (feature introduction date vs. PR merge date) so dates are consistent across the codebase.
| - Deprecation notes should mark the date as 2 years from now | |
| - Deprecation notes must use the wording `will be removed on or after YYYY-MM-DD`, where the date is 2 years after the PR merge date. |
Copilot uses AI. Check for mistakes.
| ### React template | ||
| ### React | ||
|
|
||
| - Always add `displayName` |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new rule “Always add displayName” appears inconsistent with existing React components in the repo (e.g., packages/component/src/Activity/Bubble.tsx exports memo(Bubble) without setting Bubble.displayName). Consider narrowing this to cases where displayName would otherwise be lost/unclear (e.g., anonymous functions, forwardRef, HOCs), or rephrasing as a recommendation rather than an absolute rule.
| - Always add `displayName` | |
| - For components where the inferred name would be lost or unclear (e.g. anonymous functions, `forwardRef`, HOCs such as `memo`), explicitly set `displayName` to aid debugging |
Copilot uses AI. Check for mistakes.
Comment on lines +111 to +112
| - MUST NOT modify/update any existing snapshots, let human review the test failures, they could be failing legitimately | ||
| - MUST NOT use `-u` to update snapshots, delete the snapshots and rerun `npm test` instead |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snapshot guidance is internally inconsistent: “MUST NOT modify/update snapshots” but also “delete the snapshots and rerun” (which effectively regenerates them). This also doesn’t align with .github/CONTRIBUTING.md, which instructs taking snapshots by running a scoped npm test -- --testPathPattern .... Consider clarifying the intended workflow (e.g., update only snapshots for tests you changed, always run with a scoped pattern, and ensure a human reviews diffs) rather than prohibiting -u outright.
| - MUST NOT modify/update any existing snapshots, let human review the test failures, they could be failing legitimately | |
| - MUST NOT use `-u` to update snapshots, delete the snapshots and rerun `npm test` instead | |
| - Only update snapshots for tests you intentionally changed, and always run `npm test -- --testPathPattern <test-html-file.html>` scoped to those tests | |
| - Do not run a blanket `npm test -u`; ensure a human reviews and approves all snapshot diffs before committing them |
Copilot uses AI. Check for mistakes.
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