test-anti-patterns

# Test Anti-Pattern Detection Quick, pragmatic analysis of test code in any supported language for anti-patterns and quality issues that undermine test reliability, maintainability, and diagnostic value. > **Language-specific guidance**: Try `test-analysis-extensions` once. If it is > unavailable, continue immediately with this skill's built-in framework rules; > never block the audit on the helper. ## When to Use - User asks to review test quality or find test smells - User wants to know why tests are flaky or unreliable - User asks "are my tests good?" or "what's wrong with my tests?" - User requests a test audit or test code review - User wants diagnostic findings before deciding what to improve ## When Not to Use - User wants to write new tests from scratch (use `code-testing`) - User wants direct implementation fixes rather than a diagnostic review (use the relevant write/edit skill) - User asks to fix swapped `Assert.AreEqual` argument order in MSTest (use `writing-mstest-tests`) - User asks to convert MSTest `DynamicData` from `IEnumerable<object[]>` to `ValueTuple` (use `writing-mstest-tests`) - User wants to run or execute tests (use `run-tests` for .NET) - User wants to migrate between test frameworks or versions (report that the migration workflow is not installed in this repository; do not redirect to removed skills or perform a migration during the audit) - User wants raw .NET coverage collection (use `run-tests`), non-.NET coverage collection or analysis (use native tooling), project-wide .NET coverage/CRAP metrics (use `coverage-analysis`), or named-target .NET CRAP (use `crap-score`) - User asks whether tests would catch a bug or wants behavioral/pseudo-mutation gaps (use `test-gap-analysis`) - User wants test-mix or happy-vs-error-path classification, standardized tagging, or trait/category distributions (use `test-tagging`) - User wants a deep formal test smell audit with academic taxonomy and extended catalog (use `test-smell-detection`) ## Inputs | Input | Required | Description | |-------|----------|-------------| | Test scope | No | Test files, classes, directory, or project to analyze. Discover from the current workspace when omitted. | | Production code | No | The code under test, for context on what tests should verify | | Specific concern | No | A focused area like "flakiness" or "naming" to narrow the review | ## Workflow ### Step 1: Detect language and load extension Resolve the named test path from the current workspace before asking for input. When no path is supplied, discover test files under the current directory using the repository manifests and conventional test markers. The skill context's `Base directory` is documentation storage, not the user's workspace; never resolve target files relative to it. If one reader says a path is missing but a workspace glob/search finds it, normalize that exact path and retry. Use a shell text reader (`sed`/`cat` on Unix, `Get-Content` on PowerShell) only for a confirmed reader availability, transport, or path-normalization failure and only after verifying the canonical path remains inside the current workspace. Stop on content-exclusion, permission/policy, workspace-boundary, or unknown failures. Audit any discovered file that a permitted reader can access; never ask the user to paste it. If every permitted reader fails, report the exact blocker without bypassing security boundaries. Identify the language and framework. Try the matching `test-analysis-extensions` guidance once; if unavailable, use the catalog below. ### Step 2: Gather the test code Inventory the resolved scope before reading bodies. For one file or class, read that scope directly. For a project or suite, discover test files once, batch independent reads where tools allow, and stop when every discovered test and class-level fixture has a ledger disposition. Use extension discovery markers when loaded; otherwise use the built-in markers in this skill (attributes such as `[TestClass]`/`[Fact]`/`[Test]`, `test_*.py`, `*.test.*`, `*_test.go`, `*_spec.rb`, `#[test]`, `*.Tests.ps1`, `TEST(...)`, and `TEST_CASE(...)`). Do not read unrelated production code wholesale. Open the production symbol corresponding to every suspicious test needed to decide whether an assertion, transformation, identity contract, or adjacent gap is real. For a systematic facade/surface-area pattern, every invoked member is relevant: read the entire small production type or inspect each invoked member, then map each weak test to the exact observable result, exception, state change, or boundary it should verify. ### Step 3: Scan for anti-patterns Check each test file against the anti-pattern catalog below. Report findings grouped by severity. Use extension mappings when loaded; otherwise use the cross-framework examples in the catalog. Before drafting the report, make a private completeness ledger with one row for every test method and every class-level fixture/resource. Record its oracle (or absence), exception handling, state/time dependencies, concurrency safety, precondition/assertion order, and disposition. Do not publish until every row is either attached to a finding or explicitly judged sound. In particular: - `actual != oldValue` is a weak mutation oracle: it accepts every wrong new value. Require the exact expected value. - Include unused or undisposed class-level resources; method-only scans miss fields such as a static `HttpClient`. - Treat an unsynchronized static/global collection as both order-coupled and parallel-unsafe when tests read and write it. Also flag dereferencing a nullable result before the assertion intended to prove it non-null. - When production code is supplied, note obvious untested contracts adjacent to a finding, but do not perform exhaustive branch or mutation analysis. Route that broader question to `test-gap-analysis`. #### Critical -- Tests that give false confidence | Anti-Pattern | What to Look For | |---|---| | **No assertions** | Test methods that execute code but never assert anything. A passing test without assertions proves nothing. In .NET look for missing `Assert.*`; in pytest a function with no `assert` and no `pytest.raises`; in Jest no `expect(...)`; in JUnit no `assert*`/`assertThat`; in Go a test that never calls `t.Error*`, `t.Fatal*`, or testify; in RSpec a block with no `expect`; in Pester no `Should`. Mock-call verifications (`verify(mock)`, `expect(mock).toHaveBeenCalled`, `Should -Invoke`) are real assertions. | | **Missing await on async assertions (JS/TS, .NET, Python, Kotlin, Swift)** | `expect(promise).resolves.toBe(x)` without `await`/`return`, `pytest-asyncio` test with un-awaited coroutine, `async Task` xUnit test calling `Assert.ThrowsAsync` without `await`, Kotest suspending test without `runTest`, Swift Testing async test without `await`. These tests silently pass even when the underlying assertion would have failed. | | **Coverage touching** | Test class that methodically calls every public member on a type — often in alphabetical or declaration order — without asserting meaningful outcomes. Each test typically does `var result = sut.MethodName(...)` (or `result = sut.method_name(...)`, `sut.methodName()`, `sut.MethodName(t)`) with no assertion, or only a trivial null/None/nil check. The intent is to inflate code-coverage metrics rather than verify behavior. Distinct from a single assertion-free test: the pattern is *systematic* coverage of the surface area with no real verification. | | **Self-referential assertion** | The expected value is computed from the same actual value, such as `Assert.AreEqual(dto.Name, dto.Name)`, `Assert.AreEqual(result, result)`, or equivalents. Do not apply this label merely because a valid identity, clone, serialization, or round-trip contract compares output with input: those assertions can fail. Instead check whether the input exercises a transformation and whether independently known representation, field, reference-identity, or invalid-input assertions are missing. | | **Swallowed exceptions** | `try { ... } catch { }`, `catch (Exception)` without rethrowing or asserting (.NET); bare `except:` or `except Exception:` with `pass` (Python); `try { ... } catch (e) {}` (JS/TS/Java); `defer recover()` without re-panic and no assertion (Go); `rescue StandardError` with no assertion (Ruby); `Result::unwrap_or(...)` swallowing errors in a test (Rust); empty `catch` block (Kotlin/Swift). | | **Assert in catch block only** | `try { Act(); } catch (Exception ex) { Assert.Fail(ex.Message); }` (and equivalents in other languages) -- use `Assert.ThrowsException` / `pytest.raises` / `expect(fn).toThrow` / `assertThrows` / `assert.Error(t, err)` / `#[should_panic]` / `Should -Throw` / `EXPECT_THROW` instead. The test passes when no exception is thrown even if the result is wrong. | | **Always-true assertions** | `Assert.IsTrue(true)`, `Assert.AreEqual(x, x)`, `assert True`, `expect(true).toBe(true)`, `assert.True(t, true)`, `assert!(true)`, or conditions that can never fail. | | **Commented-out assertions** | Assertions that were disabled but the test still runs, giving the illusion of coverage. | #### High -- Tests likely to cause pain | Anti-Pattern | What to Look For | |---|---| | **Flakiness indicators** | Wall-clock sleeps/waits used for synchronization: `Thread.Sleep` / `Task.Delay` (.NET), `time.sleep` (Python), `setTimeout` / `await new Promise(r => setTimeout(...))` (JS/TS), `Thread.sleep` (Java/Kotlin), `time.Sleep` (Go), `sleep` (Ruby/Bash), `std::thread::sleep` (Rust), `Start-Sleep` (Pester), `std::this_thread::sleep_for` (C++). Wall-clock reads without abstraction: `DateTime.Now`/`UtcNow`, `datetime.now()`/`datetime.utcnow()`, `Date.now()` / `new Date()`, `System.currentTimeMillis()`, `time.Now()`, `Time.now`, `Instant::now()`, `Date()`/`Date.now`, `Get-Date`, `std::chrono::system_clock::now`. Unseeded randomness: `new Random()`, `random.random()`/`random.randint()`, `Math.random()`, `new Random()` (Java/Kotlin), `rand.Int()` without seed, `rand` (Ruby), `rand::random()` (Rust). Environment-dependent paths (hard-coded `C:\...`, `/tmp/...`, network hosts). | | **Test ordering dependency** | Static/global mutable state modified across tests; setup that doesn't fully reset state (`[TestInitialize]`, `setUp`, `beforeEach`, `before(:each)`, `BeforeEach`, `t.Cleanup`); tests that fail when run individually but pass in suite (or vice versa). Examples per language: `static` fields (.NET/Java), module-level globals (Python), top-level `let`/`const` in test file (JS/TS), `var` package globals (Go), class variables (Ruby), `static mut`/`lazy_static!`/`OnceCell` (Rust), `$script:` variables (PowerShell). | | **Over-mocking** | More mock setup lines than actual test logic. Verifying exact call sequences on mocks rather than outcomes. Mocking types the test owns. Per language: Moq/NSubstitute/FakeItEasy (.NET), `unittest.mock` / `pytest-mock` (Python), Jest auto-mocks / Sinon (JS/TS), Mockito/PowerMock (Java), gomock/testify mock (Go), RSpec mocks/mocha (Ruby), `mockall` (Rust), MockK (Kotlin), `Mock` cmdlet (Pester), gmock (C++). For a deep mock audit in .NET, use `exp-mock-usage-analysis`. | | **Implementation coupling** | Testing private methods via reflection (`MethodInfo.Invoke`, `getattr` in Python, `(thing as any)` in TS, `Field.setAccessible(true)` in Java, `Object#send` in Ruby, internal `pub(crate)` access in Rust). Asserting on internal state instead of observable behavior. Verifying exact method call counts on collaborators instead of business outcomes. | | **Broad exception assertions** | `Assert.ThrowsException<Exception>(...)` (.NET) / `pytest.raises(Exception)` / `expect(fn).toThrow(Error)` without a message matcher / `assertThrows(Exception.class, ...)` (Java) / `assert.Error(t, err)` without checking the kind / `expect { ... }.to raise_error` without class (RSpec) / `#[should_panic]` without `expected = "..."` / `Should -Throw` without `-ExpectedMessage` / `EXPECT_ANY_THROW` instead of `EXPECT_THROW(stmt, SpecificType)`. | | **Weak transformation oracle** | A normalization, casing, trimming, mapping, or conversion test supplies an input already in the expected form, so a no-op implementation passes even though the assertion may catch other defects. Use an input that must change and assert an independently derived expected value. A producer/consumer round trip is useful but does not replace an independent format assertion when both sides could share the same defect. | #### Medium -- Maintainability and clarity issues | Anti-Pattern | What to Look For | |---|---| | **Poor naming** | Test names like `Test1`, `TestMethod`, or `test` that don't describe the scenario or outcome. Use the loaded extension when available; otherwise follow the existing naming convention in the same suite. | | **Magic values** | Unexplained numbers or strings in arrange/assert: `Assert.AreEqual(42, result)` / `assert result == 42` / `expect(result).toBe(42)` -- what does 42 mean? | | **Duplicate tests** | Three or more test methods with near-identical bodies that differ only in a single input value. Should be parametrized: `[DataRow]`/`[Theory]`/`[TestCase]` (.NET), `@pytest.mark.parametrize` (pytest), `test.each` / `it.each` (Jest/Vitest), `@ParameterizedTest` + `@ValueSource` (JUnit 5), `@DataProvider` (TestNG), Go table-driven tests, `where` / shared examples (RSpec), `#[rstest]` (Rust), `@ParameterizedTest` + `@MethodSource` (Kotlin), `-ForEach` / `-TestCases` (Pester), `INSTANTIATE_TEST_SUITE_P` (GoogleTest), `SECTION` / `GENERATE` (Catch2), `TEST_CASE_TEMPLATE` (doctest). For a detailed duplication analysis in .NET, use `exp-test-maintainability`. Note: Two tests covering distinct boundary conditions (e.g., zero vs. negative) are NOT duplicates -- separate tests for different edge cases provide clearer failure diagnostics and are a valid practice. | | **Giant tests** | Test methods exceeding ~30 lines or testing multiple behaviors at once. Hard to diagnose when they fail. | | **Assertion messages that repeat the assertion** | `Assert.AreEqual(expected, actual, "Expected and actual are not equal")` / `assert x == y, "x is not equal to y"` / `assertEquals(x, y, "values not equal")` add no information. Messages should describe the business meaning. | | **Missing AAA / Given-When-Then separation** | Arrange/Act/Assert (or Given/When/Then for BDD frameworks like RSpec, Kotest behavior specs, Pester) phases are interleaved or indistinguishable. | #### Low -- Style and hygiene | Anti-Pattern | What to Look For | |---|---| | **Unused test infrastructure** | Setup/teardown hooks that do nothing — `[TestInitialize]`/`[SetUp]`/`[BeforeEach]`, `setUp`/`@BeforeEach`/`@BeforeAll`, `beforeEach`/`beforeAll`, `before(:each)`/`before(:all)`, `BeforeEach`/`BeforeAll` (Pester), `setUpWithError` (XCTest) — and test helper methods that are never called. | | **Unmanaged resources** | Test creates disposable/closeable resources without cleanup: `HttpClient`/`Stream` without `using` (.NET), file/connection without `with` block or `try/finally` (Python), `FileInputStream` without `try-with-resources` (Java), `defer file.Close()` missing (Go), connection without `ensure` (Ruby), `Drop` not relied on / forgotten `close` (Rust), missing teardown for temp files / DBs in any language. |