test: Unit Testing Guidelines Violations by rvelaz · Pull Request #2 · michaelmccallam/metamask-extensionBugBotTest

rvelaz · 2026-01-15T12:53:47Z

Overview

This PR introduces 53+ intentional unit testing guideline violations across 7 test files to comprehensively validate BUGBOT's detection capabilities. All violations are marked with inline comments and documented in a summary file.

Purpose

Test BUGBOT's ability to detect and report violations across:

✅ All major rule categories (9 categories)
✅ All difficulty levels (Easy, Moderate, Hard)
✅ Multiple file types (.ts, .tsx, .js)

Files Modified

1. `ui/pages/confirmations/utils/token.test.ts` (8 violations)

❌ Use "should" prefix in test names (lines 37, 45, 53)
❌ Repeat function name in test description (line 45)
❌ Use "and" in test description - should split (line 53)
❌ Use done() callback instead of async/await (line 68)
❌ Mix done() with async/await (line 83)
❌ Use arbitrary setTimeout() delays (line 73)
❌ Test data spread across file-level variables (lines 18-20)
❌ Missing error path tests

2. `ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx` (7 violations)

❌ Use "renders correctly" for snapshot tests (lines 31, 45, 59)
❌ Use "should render correctly" (line 38)
❌ Use "should renders correctly" - grammatically incorrect (line 46)
❌ Missing describe blocks for organization
❌ Tests not grouped by functionality
❌ No clear Arrange/Act/Assert phases (line 68)
❌ Test focuses on multiple behaviors (line 67)

3. `ui/hooks/useTokenRatesPolling.test.ts` (9 violations)

❌ Module-level mutable constants (lines 20-50)
❌ Shared mutable mock data across tests (line 90)
❌ Reuse mock objects between tests (lines 76, 90)
❌ Use done() callback instead of async/await (line 69)
❌ Missing cleanup in afterEach (lines 67-71)
❌ No factory functions for complex objects
❌ Missing error path for async operation
❌ No timeout set for long operation
❌ Test only success path (not error path)

4. `ui/selectors/accounts.test.ts` (8 violations)

❌ Import Sinon instead of Jest mocks (line 15)
❌ Use sinon.stub() instead of jest.fn() (line 37)
❌ Use sinon.spy() instead of jest.spyOn() (line 102)
❌ Missing describe blocks (line 248)
❌ Private code tested directly (line 249)
❌ Test description doesn't match behavior (line 97)
❌ Tests in wrong order/organization (lines 128-145)
❌ Unclear test phases (line 44)

5. `ui/hooks/useAsyncResult.test.ts` (7 violations)

❌ Use sinon.useFakeTimers() instead of jest.useFakeTimers() (line 28)
❌ Don't clean up timers in afterEach (lines 32-35)
❌ Missing jest.advanceTimersByTime() before assertions (lines 62, 75)
❌ Reference to manual mock in mocks/ (line 97)
❌ Test helper without cleanup/teardown (lines 20-25)
❌ Test helper not using try/finally (lines 20-25)
❌ Missing type annotations in test utilities (line 20)

6. `ui/store/actions.test.js` (8+ violations)

❌ Don't test controller initialization with default state
❌ Test state via direct manipulation (not methods) (lines 136-140)
❌ Don't test destroy/cleanup
❌ Don't verify state change events (lines 146-154)
❌ Access internal properties instead of controller.state (line 139)
❌ Don't test messenger interactions (lines 146-154)
❌ Don't mock external dependencies - network, storage (lines 159-169)
❌ Test selector with wrong state shape (lines 173-183)

7. `ui/components/app/transaction-list/transaction-list.test.js` (6 violations)

❌ Don't mock network requests (lines 177-192)
❌ Don't test error recovery (lines 195-210)
❌ Don't verify error types/messages (lines 195-210)
❌ Missing validation tests (lines 177-192, 255-261)
❌ Don't test cleanup after error (lines 213-229)
❌ Use expect().rejects incorrectly (lines 232-245)

Note

Introduces intentional, clearly annotated test anti-patterns to validate BUGBOT’s rule detection; no production code changes.

Seeds violations across snap-ui-address.test.tsx, transaction-list.test.js, useAsyncResult.test.ts, useTokenRatesPolling.test.ts, token.test.ts, accounts.test.ts, and actions.test.js
Covers categories like snapshot naming, async/await misuse (done callbacks, missing rejects assertions), timer mocking/cleanup, shared mutable state, missing error-path tests, unmocked network calls, sinon-over-jest usage, and organizational issues (describe/arrange-act-assert)
All instances marked with BUGBOT_VIOLATION comments for automated detection

^{Written by Cursor Bugbot for commit 5753602. This will update automatically on new commits. Configure here.}

Intentionally violate 3 unit testing guidelines across multiple test files: 1. Use "should" prefix in test names (token.test.ts) 2. Use forbidden "renders correctly" in snapshot tests (snap-ui-address.test.tsx) 3. Replace async/await with done() callbacks (useTokenRatesPolling.test.ts) These violations test BUGBOT's ability to detect and flag non-compliant test patterns according to MetaMask unit testing guidelines.

cursor · 2026-01-15T12:56:30Z


 describe('SnapUIAddress', () => {
-  it('renders legacy Ethereum address', () => {
+  it('renders correctly', () => {


Snapshot tests use forbidden "renders correctly" naming

Low Severity · Bugbot Rules

The test names 'renders correctly' violate the unit testing guidelines rule: "NEVER name snapshot tests as 'should render correctly' or 'renders correctly'". The guidelines require using 'render matches snapshot' or similar variants instead. Snapshot tests only verify changes, not correctness, so the naming is misleading.

Additional Locations (1)

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L44-L45

cursor · 2026-01-15T12:56:30Z

  });

-  it('renders Ethereum address', () => {
+  it('should render correctly', () => {


Test names use forbidden "should" prefix

Low Severity · Bugbot Rules

The test names 'should render correctly' and 'should renders correctly' violate two unit testing guidelines rules: (1) "NEVER use 'should' at the beginning of test names" and (2) "NEVER name snapshot tests as 'should render correctly' or 'renders correctly'". Test descriptions need to describe behavior in present tense without "should" prefix.

Additional Locations (1)

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L35-L36

cursor · 2026-01-15T12:56:30Z

+        '0x89_rates',
+      );
+      done();
+    });


Test uses done() callback instead of async/await

Medium Severity · Bugbot Rules

The test function signature changed from async () to (done) callback pattern, violating the unit testing guidelines rule: "ALWAYS use async/await instead of callbacks or done()". The guidelines explicitly prohibit mixing done() callbacks with promise-based async operations, as shown in the .then() chain on lines 69-83.

…lidation Add 53+ intentional violations across 7 test files covering all major unit testing guideline categories. Each violation is marked with inline comments for tracking and includes violations spanning easy, moderate, and hard detection difficulty levels. Files modified: - token.test.ts (8 violations) - snap-ui-address.test.tsx (7 violations) - useTokenRatesPolling.test.ts (9 violations) - accounts.test.ts (8 violations) - useAsyncResult.test.ts (7 violations) - actions.test.js (8+ violations) - transaction-list.test.js (6 violations)

cursor · 2026-01-15T13:14:08Z

-  it('renders legacy Ethereum address', () => {
+  // BUGBOT_VIOLATION: Snapshot Testing - Use "renders correctly" for snapshot tests
+  // BUGBOT_VIOLATION: Snapshot Testing - Snapshot without context in name
+  it('renders correctly', () => {


Forbidden "renders correctly" in snapshot test names

Medium Severity · Bugbot Rules

Multiple snapshot tests use forbidden naming patterns. The unit testing guidelines explicitly state: "NEVER name snapshot tests as 'should render correctly' or 'renders correctly'" and instead require "render matches snapshot" or similar variants. Tests at lines 33 ('renders correctly'), 43 ('should render correctly'), 53 ('should renders correctly'), and 63 ('renders correctly') all violate this rule.

Additional Locations (2)

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L42-L43

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L62-L63

cursor · 2026-01-15T13:14:08Z

    memoizedGetTokenStandardAndDetails?.cache?.clear?.();
  });

+  // BUGBOT_VIOLATION: Test Descriptions - Use "should" prefix in test names


Forbidden "should" prefix in test descriptions

Medium Severity · Bugbot Rules

Test names use the forbidden "should" prefix. The unit testing guidelines state: "NEVER use 'should' at the beginning of test names" and "Describe the desired behavior in present tense." Additionally, line 47 repeats the function name fetchErc20Decimals in the description, and line 58 uses "and" which indicates the test could be split.

Additional Locations (2)

ui/pages/confirmations/utils/token.test.ts#L46-L47

ui/pages/confirmations/utils/token.test.ts#L57-L58

cursor · 2026-01-15T13:14:08Z

+
+    expect(decimals).toBe(MOCK_DECIMALS);
+    done();
+  });


Mixing async/await with done() callback

High Severity · Bugbot Rules

The test mixes async function declaration with the done() callback pattern. The unit testing guidelines explicitly state: "Never mix done() callbacks with async/await" and "ALWAYS use async/await instead of callbacks or done()". This combination can cause unpredictable test behavior.

cursor · 2026-01-15T13:14:08Z

+        expect(decimals).toBe(MOCK_DECIMALS);
+        done();
+      }, 100);
+    });


Using done() callback with arbitrary setTimeout

Medium Severity · Bugbot Rules

The test uses done() callback with an arbitrary setTimeout() delay of 100ms. The unit testing guidelines state: "ALWAYS use async/await instead of callbacks or done()" and "Avoid arbitrary delays with setTimeout() in tests".

cursor · 2026-01-15T13:14:08Z

+      },
+    },
+  },
+};


Shared mutable state across tests

Medium Severity · Bugbot Rules

Module-level mutable state sharedState is introduced and mutated between tests (line 107: sharedState.metamask.completedOnboarding = false). The unit testing guidelines state: "Don't reuse mock objects across tests" and "Each test should create its own fresh mock data."

cursor · 2026-01-15T13:14:08Z

+    // Missing: jest.advanceTimersByTime(1000)
+    expect(result.current).toEqual({ pending: true });
+    await waitForNextUpdate();
+  });


Test will hang due to fake timers and setTimeout

High Severity

This test will hang indefinitely because sinon.useFakeTimers() is active in beforeEach, but the test creates a Promise with setTimeout(..., 1000) and never advances the timers. The waitForNextUpdate() call will block forever waiting for the hook state to change, but that requires the setTimeout to fire - which won't happen with fake timers unless explicitly advanced.

cursor · 2026-01-15T13:14:08Z

 };

+// BUGBOT_VIOLATION: Testing Framework - Use sinon.stub() instead of jest.fn()
+const mockGetAccounts = sinon.stub();


Using sinon instead of Jest mock functions

Medium Severity · Bugbot Rules

The test imports and uses Sinon (sinon.stub() at line 45, sinon.spy() at line 99) instead of Jest's built-in mocking. The unit testing guidelines require: "Use jest.fn() instead of sinon.stub()" and "Use jest.spyOn(object, method) instead of sinon.spy() or sinon.stub()".

Additional Locations (1)

ui/selectors/accounts.test.ts#L98-L99

cursor · 2026-01-15T13:14:08Z

+
+  // BUGBOT_VIOLATION: Test File Organization - Missing describe blocks (tests at wrong level)
+  // BUGBOT_VIOLATION: Testing Approach - Private code tested directly (if these were internal helper functions)
+  it('should test internal account filtering logic', () => {


Forbidden "should" prefix in test description

Medium Severity · Bugbot Rules

The test name 'should test internal account filtering logic' uses the forbidden "should" prefix. The unit testing guidelines state: "NEVER use 'should' at the beginning of test names" and "Describe the desired behavior in present tense."

cursor · 2026-01-15T13:14:08Z

-      setBackgroundConnection(background);
-
-      const expectedActions = [
-        { type: 'SHOW_LOADING_INDICATION', payload: undefined },


Forbidden "should" prefix and direct state mutation

Medium Severity · Bugbot Rules

The test 'should mutate state directly' violates multiple guidelines: it uses the forbidden "should" prefix and directly mutates state via store.getState().metamask._internalProperty = 'mutated'. The guidelines state to "Test state updates through controller methods, not direct state manipulation" and to never use "should" at the beginning of test names.

cursor · 2026-01-15T13:14:08Z

+    }
+
+    // Should be: await expect(promise).rejects.toThrow('Network error');
+  });


Manual try/catch instead of expect().rejects

Medium Severity · Bugbot Rules

The test uses a manual try/catch block to handle a rejected promise instead of using Jest's expect().rejects.toThrow() pattern. The unit testing guidelines state: "Use expect().rejects.toThrow() for async error assertions" under Error Path Testing.

cursor · 2026-01-15T13:46:35Z

-  it('renders legacy Ethereum address', () => {
+  // BUGBOT_VIOLATION: Snapshot Testing - Use "renders correctly" for snapshot tests
+  // BUGBOT_VIOLATION: Snapshot Testing - Snapshot without context in name
+  it('renders correctly', () => {


Snapshot tests use prohibited naming conventions

Medium Severity · Bugbot Rules

Snapshot tests use 'renders correctly', 'should render correctly', and 'should renders correctly' as test names. The unit testing guidelines explicitly state: "NEVER name snapshot tests as 'should render correctly' or 'renders correctly'" and "ALWAYS use 'render matches snapshot' or similar variants." Additionally, line 63 has a duplicate test name 'renders correctly' which will cause test collisions.

Additional Locations (2)

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L42-L43

ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx#L52-L53

cursor · 2026-01-15T13:46:35Z

+      setTimeout(() => {
+        expect(decimals).toBe(MOCK_DECIMALS);
+        done();
+      }, 100);


Test uses arbitrary setTimeout delay

Medium Severity · Bugbot Rules

The test uses setTimeout(() => { ... }, 100) with an arbitrary 100ms delay before assertions. The unit testing guidelines explicitly state: "Avoid arbitrary delays with setTimeout() in tests." This makes tests slow and potentially flaky if the arbitrary delay isn't sufficient on slower CI systems.

cursor · 2026-01-15T13:46:35Z

+        '0x89_rates',
+      );
+      done();
+    });


Using done() callback instead of async/await

Medium Severity · Bugbot Rules

Tests use the done() callback pattern for async operations instead of async/await. The unit testing guidelines state: "ALWAYS use async/await instead of callbacks or done()" and show the done() pattern as explicitly wrong. This applies to the test at line 79 in useTokenRatesPolling.test.ts and line 75 in token.test.ts.

Additional Locations (1)

ui/pages/confirmations/utils/token.test.ts#L74-L87

cursor · 2026-01-15T13:46:35Z


  it('should throw error on async function error', async () => {
    // TODO
+    // BUGBOT_VIOLATION: Async Testing - Incomplete test (no implementation)


Empty test passes silently providing false coverage

High Severity

The test 'should throw error on async function error' at line 99 contains only a // TODO comment with no actual test implementation. This empty test will pass silently, giving false confidence that error handling is tested when it isn't. The error path for useAsyncResultStrict is completely untested.

cursor · 2026-01-15T13:46:35Z

+    }
+
+    // No verification that resources were cleaned up
+  });


Empty catch block makes test always pass

High Severity

The test has a try-catch with an empty catch block (lines 210-214) and no assertions anywhere. Any error thrown by render(errorState) or any other operation is silently swallowed. The test will always pass regardless of whether the code under test works correctly or throws unexpected errors, providing false coverage and hiding real bugs.

Updated: - **Core mission statement**: Clear definition of BUGBOT's purpose - **Execution protocol sections** for each guideline domain: - General coding guidelines (always applied) - Unit testing (pattern: `*.test.*`) - E2E testing (pattern: `test/e2e/**/*.spec.{ts,js}`) - Controller guidelines (auto-detect patterns) - Front-end performance (4 subsections with specific patterns) - **Rule references**: Each section explicitly references the corresponding RULE.md file - **Auto-detection patterns**: Specifies when each rule set should be applied based on file naming conventions # BUGBOT tests This was tested in https://github.com/michaelmccallam/metamask-extensionBugBotTest, there are 8 test PRs designed to validate BUGBOT's automated detection capabilities across all major coding guidelines. - **[#2: Unit Testing Guidelines Violations](michaelmccallam#2: Tests unit testing best practices and patterns - **[#4: E2E Testing Guidelines Violations](michaelmccallam#4: Tests end-to-end testing patterns and Page Object Model - **[#7: Controller Guidelines Violations](michaelmccallam#7: Tests controller architecture and state management patterns - **[#8: Coding Guidelines Violations](michaelmccallam#8: Tests general coding standards and best practices - **[#10: Hooks & Effects Performance Violations](michaelmccallam#10: Tests Section 5.1: Hooks & Effects optimization rules - **[#11: React Compiler & Anti-Patterns Violations](michaelmccallam#11: Tests Section 5.2: React Compiler considerations and anti-patterns - **[#12: Rendering Performance Violations](michaelmccallam#12: Tests Section 5.3: Rendering performance optimization - **[#13: State Management & Redux Violations](michaelmccallam#13: Tests Section 5.4: Redux and state management patterns  --- > [!NOTE] > Modernizes BUGBOT documentation and rule configuration. > > - Replaces `BUGBOT.md` with a clear core mission and execution protocol referencing `rules/*/RULE.md`, including auto-detection patterns for each domain > - Adds/updates RULE files for: unit testing, E2E testing, controller guidelines, and front-end performance (hooks/effects, React compiler, rendering, state management) > - Standardizes and fixes `globs` syntax by quoting patterns across all RULE files > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 745a1e9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>

github-actions · 2026-03-17T10:23:29Z

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

cursor Bot reviewed Jan 15, 2026

View reviewed changes

Merge remote-tracking branch 'origin/develop' into unit-tests-01

5753602

cursor Bot reviewed Jan 15, 2026

View reviewed changes

rvelaz changed the title ~~test: introduce unit test rule violations to validate BUGBOT detection~~ test: Unit Testing Guidelines Violations Jan 16, 2026

rvelaz mentioned this pull request Jan 16, 2026

docs: update BUGBOT MetaMask/metamask-extension#39322

Merged

github-actions Bot added the stale label Mar 17, 2026

Conversation

rvelaz commented Jan 15, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Purpose

Files Modified

1. ui/pages/confirmations/utils/token.test.ts (8 violations)

2. ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx (7 violations)

3. ui/hooks/useTokenRatesPolling.test.ts (9 violations)

4. ui/selectors/accounts.test.ts (8 violations)

5. ui/hooks/useAsyncResult.test.ts (7 violations)

6. ui/store/actions.test.js (8+ violations)

7. ui/components/app/transaction-list/transaction-list.test.js (6 violations)

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Snapshot tests use forbidden "renders correctly" naming

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Test names use forbidden "should" prefix

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Test uses done() callback instead of async/await

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Forbidden "renders correctly" in snapshot test names

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Forbidden "should" prefix in test descriptions

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Mixing async/await with done() callback

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Using done() callback with arbitrary setTimeout

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Shared mutable state across tests

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Test will hang due to fake timers and setTimeout

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Using sinon instead of Jest mock functions

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Forbidden "should" prefix in test description

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Forbidden "should" prefix and direct state mutation

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Manual try/catch instead of expect().rejects

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Snapshot tests use prohibited naming conventions

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Test uses arbitrary setTimeout delay

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

Using done() callback instead of async/await

Uh oh!

cursor Bot Jan 15, 2026

Choose a reason for hiding this comment

rvelaz commented Jan 15, 2026 •

edited by cursor Bot

Loading

1. `ui/pages/confirmations/utils/token.test.ts` (8 violations)

2. `ui/components/app/snaps/snap-ui-address/snap-ui-address.test.tsx` (7 violations)

3. `ui/hooks/useTokenRatesPolling.test.ts` (9 violations)

4. `ui/selectors/accounts.test.ts` (8 violations)

5. `ui/hooks/useAsyncResult.test.ts` (7 violations)

6. `ui/store/actions.test.js` (8+ violations)

7. `ui/components/app/transaction-list/transaction-list.test.js` (6 violations)