diff --git a/CHANGELOG.md b/CHANGELOG.md index 6659f982..5bb5487b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `feature-flags` skill with a repo-agnostic base and a MetaMask Mobile overlay for version-gated remote flags. Marked `base: true` so it installs even when its domain is filtered out. ([#147](https://github.com/MetaMask/skills/pull/147)) - Add `analytics` skill (`platform/analytics`, moved from `coding`) with a repo-agnostic base and a MetaMask Mobile overlay for the canonical tracking API. Marked `base: true` so it installs even when its domain is filtered out. ([#140](https://github.com/MetaMask/skills/pull/140)) +### Changed + +- Stop testing skills from prompting unit tests for static styling. Mobile and Extension layer policies now treat spacing, color, typography, and fixed layout props as GAP / ACCEPT, the unit references ban style-value assertions and style-only test IDs, and coding guidelines no longer require tests for every component edit. + ## [0.3.1] ### Fixed diff --git a/domains/coding/skills/coding-guidelines/repos/metamask-extension.md b/domains/coding/skills/coding-guidelines/repos/metamask-extension.md index 508d4d13..fd91fe9e 100644 --- a/domains/coding/skills/coding-guidelines/repos/metamask-extension.md +++ b/domains/coding/skills/coding-guidelines/repos/metamask-extension.md @@ -586,7 +586,8 @@ export function formatBalance( ### Write Tests for All Components and Utilities -- **ALWAYS write tests for new code** +- **ALWAYS write tests for new behavior** (logic, state, and conditional UI) +- **Do not** write tests for static styling changes (spacing, color, typography, class-name swaps); see `extension-testing` → `references/unit.md` - Tests reduce possibilities of errors and regressions - Ensure components behave as expected - Use Jest as the testing framework @@ -779,7 +780,7 @@ Before submitting a PR, ensure: ### Testing -- [ ] Unit tests written for components +- [ ] Unit tests written for component behavior (not static styling) - [ ] Unit tests written for utilities - [ ] Tests follow naming conventions - [ ] Tests cover happy paths and error cases diff --git a/domains/coding/skills/coding-guidelines/repos/metamask-mobile.md b/domains/coding/skills/coding-guidelines/repos/metamask-mobile.md index e966062b..95f7425f 100644 --- a/domains/coding/skills/coding-guidelines/repos/metamask-mobile.md +++ b/domains/coding/skills/coding-guidelines/repos/metamask-mobile.md @@ -18,7 +18,7 @@ parent: coding-guidelines **Code Quality**: - TypeScript guidelines from contributor docs • Functional components + hooks • PascalCase (components) / camelCase (functions) -- Reusable components/utilities • TSDoc format • Comprehensive tests following testing layers (below) +- Reusable components/utilities • TSDoc format • Comprehensive tests following testing layers (below) • No tests for static styling changes (spacing, color, typography, fixed layout props) - Redux selectors: install **selector-patterns** (`yarn skills --include coding/selector-patterns --save`) when writing or updating them. **Testing layers** (Mobile — canonical policy: testing domain `knowledge/testing-layers.md`, installed beside `mobile-testing`): diff --git a/domains/testing/knowledge/extension-testing-layers.md b/domains/testing/knowledge/extension-testing-layers.md index 44d20300..3e947d85 100644 --- a/domains/testing/knowledge/extension-testing-layers.md +++ b/domains/testing/knowledge/extension-testing-layers.md @@ -57,6 +57,29 @@ Is this scenario worth covering? (distinct realistic regression) └─ No → GAP / ACCEPT (do not invent E2E) ``` +## Not worth covering: static presentation + +A change to static or default presentation is **GAP / ACCEPT** at every layer. +Do not add or update a test for it. This includes: + +- Spacing, padding, margin, and alignment +- Color, typography, and design-token values +- Fixed truncation or other layout props that never change with state +- Swapping one class name, SCSS rule, or style value for another + +Do not add a `data-testid` only so a test can read `style` or `className`, and +do not add or refresh a snapshot because only class names or static styles +changed. Verify these changes with screenshots (`visual-testing`) and the PR's +before/after evidence. + +Presentation that depends on state or props is behavior. Test it, and assert +the outcome a user or caller sees (text shown or hidden, disabled, ARIA state), +not pixel or class values. + +Do not use a coverage delta as the reason to add or skip a test. Coverage can +rise from a render that asserts nothing, and a real bug fix can leave coverage +flat. Coverage reports only point at uncovered branches worth considering. + ## Defaults | Layer | Owns | Existing homes | diff --git a/domains/testing/knowledge/testing-layers.md b/domains/testing/knowledge/testing-layers.md index 66d691e8..5fbf2774 100644 --- a/domains/testing/knowledge/testing-layers.md +++ b/domains/testing/knowledge/testing-layers.md @@ -49,6 +49,28 @@ Is this scenario worth covering? (distinct realistic regression) └─ No → GAP / ACCEPT (do not invent E2E) ``` +## Not worth covering: static presentation + +A change to static or default presentation is **GAP / ACCEPT** at every layer. +Do not add or update a test for it. This includes: + +- Spacing, padding, margin, inset, and alignment +- Color, typography, and design-token values +- Unconditional `numberOfLines` / `ellipsizeMode` or other fixed layout props +- Swapping one Tailwind class or style value for another + +Do not add a `testID` only so a test can read `style` or `className`. Verify +these changes with screenshots (`mobile-visual-testing`) and the PR's +before/after evidence. + +Presentation that depends on state or props is behavior. Test it, and assert +the outcome a user or caller sees (text shown or hidden, disabled, accessibility +state), not pixel or class values. + +Do not use a coverage delta as the reason to add or skip a test. Coverage can +rise from a render that asserts nothing, and a real bug fix can leave coverage +flat. Coverage reports only point at uncovered branches worth considering. + ## Defaults | Layer | Owns | File pattern | diff --git a/domains/testing/skills/extension-testing/references/unit.md b/domains/testing/skills/extension-testing/references/unit.md index 0d5fd4bd..1a60227d 100644 --- a/domains/testing/skills/extension-testing/references/unit.md +++ b/domains/testing/skills/extension-testing/references/unit.md @@ -9,6 +9,34 @@ Reference: [MetaMask Unit Testing Guidelines](https://github.com/MetaMask/contri - **ALWAYS use Jest** for unit tests (not Mocha or Tape) - Leverage Jest's built-in features: module mocks, timer mocks, snapshots, and parallel test execution +### Do Not Test Static Presentation + +Choose the layer first via installed `knowledge/extension-testing-layers.md`. Static or default presentation is GAP / ACCEPT there, so a diff that only changes it needs no new or updated test. + +- **NEVER** assert spacing, color, typography, or fixed layout values (computed styles, `className` strings, design-token values) +- **NEVER** add or refresh a snapshot because only class names, SCSS, or static styles changed +- **NEVER** add a `data-testid` whose only use is reading `style` or `className` +- **Do not** justify a test by coverage alone. A render that touches a new JSX line adds coverage without protecting behavior. +- Verify static presentation with screenshots (`visual-testing`) and the PR's before/after evidence + +```typescript +❌ WRONG: +it('removes the extra padding from the price summary', () => { + render(); + expect(screen.getByTestId('price-summary')).not.toHaveClass('px-4'); +}); +``` + +Presentation that depends on state or props is behavior. Test it by asserting what the user sees (text shown or hidden, disabled, ARIA state), not class names or style values. + +```typescript +✅ CORRECT: +it('disables the submit button when the amount is empty', () => { + render(); + expect(screen.getByRole('button', { name: 'Send' })).toBeDisabled(); +}); +``` + ### Test File Organization #### File Placement @@ -188,6 +216,7 @@ async function withController(...args: WithControllerArgs) { - **ALWAYS use "render matches snapshot" or similar variants** - Add context when needed: "render matches snapshot when not enabled" - Remember: Snapshots only check for changes, NOT correctness +- Do not add a snapshot to cover a static styling change; see [Do Not Test Static Presentation](#do-not-test-static-presentation) Examples: @@ -748,6 +777,7 @@ Before submitting tests, ensure: ### Final Checks - [ ] Snapshot tests are named "render matches snapshot" +- [ ] No tests or snapshot updates exist only to cover static styling - [ ] All tests pass and are deterministic - [ ] Tests run in isolation (can run individually) - [ ] No console errors or warnings diff --git a/domains/testing/skills/mobile-testing/references/unit.md b/domains/testing/skills/mobile-testing/references/unit.md index 6bb85c84..457c125b 100644 --- a/domains/testing/skills/mobile-testing/references/unit.md +++ b/domains/testing/skills/mobile-testing/references/unit.md @@ -21,6 +21,35 @@ Follow installed `knowledge/testing-layers.md` before choosing unit vs component - **Default for screen/view behavior:** `*.view.test.tsx` via [`component-view.md`](component-view.md) — not a broad RTL unit test that mocks hooks/selectors. - **This skill applies when:** pure helpers, local utilities, narrow component contracts, or the CV framework cannot cover the case yet (smallest focused unit test + note why). - **Do not** add new full-page `*.test.tsx` files that render a screen and mock Redux/hooks to force UI state; convert or write CV instead. +- **Do not** add a unit test for static presentation (spacing, color, typography, fixed layout props). The layer policy marks it GAP / ACCEPT — see [Do Not Test Static Presentation](#do-not-test-static-presentation). + +## Do Not Test Static Presentation + +A diff that only changes static or default styling needs no new or updated test, even inside an existing `*.test.tsx`. These assertions do not catch regressions a user would notice, and they break whenever a design token changes. + +```tsx +// ❌ WRONG - asserts padding values after a spacing fix +const style = StyleSheet.flatten( + screen.getByTestId(SelectorsIDs.PRICE_SUMMARY).props.style, +); +expect(style.paddingLeft).toBeUndefined(); + +// ❌ WRONG - asserts fixed truncation props; Jest never renders the overflow +expect(title.props.numberOfLines).toBe(1); +expect(title.props.ellipsizeMode).toBe('tail'); + +// ❌ WRONG - asserts margins set by a Tailwind class +expect(StyleSheet.flatten(divider.props.style)).toEqual( + expect.objectContaining({ marginBottom: 0, marginTop: 20 }), +); +``` + +- **Do not** add a `testID` whose only use is reading `style`. +- **Do not** remove a `useTailwind` mock so `StyleSheet.flatten` can see real spacing. +- **Do not** justify a test by coverage alone. A render that touches a new JSX line adds coverage without protecting behavior. +- Verify static presentation with screenshots (`mobile-visual-testing`) and the PR's before/after evidence. + +Presentation that depends on state or props is behavior. Test it by asserting what the user sees (text shown or hidden, disabled, accessibility state), not style values. ## Test Naming Rules @@ -77,7 +106,7 @@ const createTestEvent = (overrides = {}) => ({ - **ALWAYS prefer `testID` props** for selecting elements in tests - **Use `getByTestId`** as the primary query method for reliable element selection -- **Add `testID` props** to components when writing new code or updating existing code +- **Add `testID` props** when a justified behavior test needs a stable query — not just because a component was touched, and never only to read `style` - **Avoid selecting by text** when the text might change (i18n, copy updates) ```tsx @@ -418,7 +447,9 @@ describe('MetaMetricsCustomTimestampPlugin', () => { ## Test Coverage (MANDATORY) -**EVERY component MUST test:** +Apply this list only after the layer policy says a unit test is justified. Cover the behavior branches the code actually has. A static styling change adds no branch, so it adds no test. + +**When a unit test is justified, it MUST cover:** - ✅ **Happy path** - normal expected behavior - ✅ **Edge cases** - null, undefined, empty values, boundary conditions @@ -597,6 +628,7 @@ expect(result).toBe(false); Before submitting any test file, verify: - [ ] **No `toMatchSnapshot()` calls** — BANNED; use explicit assertions or `toMatchInlineSnapshot()` instead +- [ ] **No static style assertions** — no `StyleSheet.flatten` padding/margin/color checks, fixed layout-prop checks, or style-only testIDs - [ ] **No mocking to inject testIDs** - Use component's built-in testID support - [ ] **testIDs via child prop objects** - Use `closeButtonProps={{ testID }}` not mocks - [ ] **No "should" in any test name** @@ -617,6 +649,7 @@ Before submitting any test file, verify: - ❌ **Using `toMatchSnapshot()`** — BANNED; it writes opaque `.snap` files with no code owner; use explicit assertions or `toMatchInlineSnapshot()` instead - ❌ **Mocking to inject testIDs** - Components already support testID (see guidelines above) +- ❌ **Testing static styling** - Spacing, color, and fixed layout props are verified visually, not in Jest - ❌ **Using "should" in test names** - This is the #1 mistake, use action-oriented descriptions - ❌ **Testing multiple behaviors in one test** - One test, one behavior - ❌ **Sharing state between tests** - Each test must be independent