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**: Call the `test-analysis-extensions` skill to discover available extension files, then read the file matching the target codebase (e.g., `extensions/dotnet.md`, `extensions/python.md`, `extensions/typescript.md`, `extensions/go.md`). The extension file tells you which sleep / time / random / skip / setup-teardown / mystery-guest APIs to look for in that language. ## 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 to improve existing test code ## When Not to Use - User wants to write new tests from scratch (use `code-testing-agent` for any language, or `writing-mstest-tests` for MSTest specifically) - 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 (use migration skills) - User wants to measure code coverage (out of scope) - User wants a deep formal test smell audit with academic taxonomy and extended catalog (use `test-smell-detection`) ## Inputs | Input | Required | Description | |-------|----------|-------------| | Test code | Yes | One or more test files or classes to analyze | | 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 Identify the target codebase's language and test framework. Call the `test-analysis-extensions` skill and read the matching extension file. The extension file documents framework-specific anti-pattern markers — what counts as a sleep/wait, a test marker, a skip, a setup/teardown, a shared-state hot spot, and an integration boundary — so this skill stays language-neutral. ### Step 2: Gather the test code Read the test files the user wants reviewed. If the user points to a directory or project, scan for all test files using the discovery markers in the loaded language extension file (e.g., `[TestClass]`/`[Fact]`/`[Test]` for .NET, `test_*.py` / `def test_*` for pytest, `*.test.ts` / `it()` for Jest, `*Test.java` / `@Test` for JUnit, `*_test.go` / `func TestXxx` for Go, `*_spec.rb` for RSpec, `#[test]` for Rust, `*.Tests.ps1` / `Describe` for Pester, `TEST(...)` for GoogleTest, `TEST_CASE(...)` for Catch2/doctest). If production code is available, read it too -- this is critical for detecting tests that are coupled to implementation details rather than behavior. ### Step 3: Scan for anti-patterns Check each test file against the anti-pattern catalog below. Report findings grouped by severity. The examples are .NET-centric but the patterns generalize — use the loaded language extension file to map each pattern to the framework you are auditing. #### 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** | Asserts that the output of an operation equals its input when the operation is expected to be an identity or no-op, e.g. `Assert.AreEqual(input, Parse(input.ToString()))`, `assert input == parse(str(input))`, `expect(parse(input.toString())).toBe(input)`, `assert.Equal(t, input, parse(input))`. Also flags `Assert.AreEqual(dto.Name, dto.Name)` / `assert dto.name == dto.name` / `expect(dto.name).toBe(dto.name)` (asserting a field against itself). The test is tautological — it can only fail if the round-trip is broken, but never verifies that a *transformation* actually happened. | | **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)`. | #### Medium -- Maintainability and clarity issues | Anti-Pattern | What to Look For | |---|---| | **Poor naming** | Test names like `Test1`, `TestMethod`, `test`, names that don't describe the scenario or expected outcome. Good naming differs by language convention — see the loaded language extension file (e.g., `Add_NegativeNumber_ThrowsArgumentException` for .NET, `test_add_negative_number_raises_value_error` for pytest, `addNegativeNumber_throwsArgumentException` for Java, `'adds negative number throws'` for Jest descriptions, `TestAdd_NegativeNumber_ReturnsError` for Go). | | **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. | | **Print debugging** | Leftover `Console.WriteLine` / `Debug.WriteLine` / `print()` / `console.log` / `System.out.println` / `fmt.Println` / `puts` / `dbg!` / `Write-Host` / `std::cout` statements used during test development. | | **Inconsistent naming convention** | Mix of naming styles in the same test class/module/file (e.g., some use `Method_Scenario_Expected`, others use `ShouldDoSomething`). | ### Step 4: Calibrate severity honestly