mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
Improve test smell skill quality and eval power (#1056)
* Improve test smell skill quality Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Use conventional empty class bodies Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Improve test smell calibration Align workspace discovery and false-positive decisions with the losing eval transcripts, correct contradictory fixtures, and make graders outcome-focused. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Make eval regexes multiline-safe Allow outcome evidence to match across line breaks in generated review output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Make notification fixtures observable Record notification identifiers so post-wait assertions can fail, while preserving fixed sleeps as the intentional smell under evaluation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Strengthen test smell stop conditions Require workspace discovery, preserve formal skip and file classifications, prevent clean-suite false positives, and reduce lexical grader coupling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Use conventional exception class body Keep the fixture compatible with compilers that do not accept semicolon-only class declarations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Remove brittle eval gates Rely on outcome rubrics instead of narrow lexical matches and keep the Sensitive Equality fixture culture-stable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Add JUnit eval exit check Fail fast on empty or failed trial output while dropping a redundant severity-word matcher. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Make remaining eval regexes multiline-safe Allow concise verdict and async-fix patterns to match wrapped model output across line breaks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b * Preserve non-catalog validity findings Keep formal smell classification while separately reporting proven test-validity defects that do not belong to the taxonomy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 718f824b-6b86-4d7f-9428-2d7a8908e95b
This commit is contained in:
Amaury Levé
@@ -1,240 +1,142 @@
|
||||
---
|
||||
name: test-smell-detection
|
||||
description: >
|
||||
Deep-dive audit using the full testsmells.org 19-smell academic catalog
|
||||
for tests in any language. Every finding maps to a named, citable smell
|
||||
from the research literature (Assertion Roulette, Duplicate Assert,
|
||||
Mystery Guest, Eager Test, Sensitive Equality, Conditional Test Logic,
|
||||
Sleepy Test, Magic Number Test, etc.) with research-backed severity.
|
||||
Polyglot: .NET (MSTest/xUnit/NUnit/TUnit), Python (pytest/unittest),
|
||||
TS/JS (Jest/Vitest/Mocha/node:test), Java (JUnit/TestNG), Go, Ruby
|
||||
(RSpec/Minitest), Rust, Swift, Kotlin (JUnit/Kotest), PowerShell
|
||||
(Pester), C++ (GoogleTest/Catch2).
|
||||
INVOKE ONLY when explicitly asked for the testsmells.org 19-smell
|
||||
academic catalog or citable smell names from the literature.
|
||||
DO NOT USE FOR: general or pragmatic audits — use test-anti-patterns;
|
||||
writing new tests (use code-testing-agent, or writing-mstest-tests for
|
||||
MSTest); running tests (use run-tests); framework migration.
|
||||
Audits existing tests in any language using formal, research-backed test
|
||||
smell names and the testsmells.org 19-smell academic taxonomy. Use when the
|
||||
caller asks for an academic or citable test-smell review, named smell
|
||||
categories, or a formal severity-ranked smell assessment. Covers Assertion
|
||||
Roulette, Conditional Test Logic, Mystery Guest, Eager Test, Sleepy Test,
|
||||
Unknown Test, Sensitive Equality, and the rest of the catalog across .NET,
|
||||
Python, JavaScript/TypeScript, Java, Go, Ruby, Rust, Swift, Kotlin,
|
||||
PowerShell, and C++. DO NOT USE FOR a quick pragmatic test review (use
|
||||
test-anti-patterns), writing or running tests, framework migration, coverage,
|
||||
or assertion-diversity metrics.
|
||||
license: MIT
|
||||
---
|
||||
|
||||
# Test Smell Detection
|
||||
|
||||
Deep formal audit of test code in any supported language using an academic test smell taxonomy. Detects symptoms of bad design or implementation decisions that make tests harder to understand, more fragile, less effective at catching bugs, or more expensive to maintain. Produces a severity-ranked report with specific locations and actionable fixes.
|
||||
Audit test code with the academic taxonomy, code evidence, calibrated
|
||||
framework idioms, and fixes native to the codebase.
|
||||
|
||||
> **Language-specific guidance**: Call the `test-analysis-extensions` skill to discover available extension files, then read the file matching the target codebase. The extension file documents test markers, sleep / time / random APIs, skip annotations, setup/teardown, mystery-guest indicators (file/database/network/env), integration markers, and language-specific calibration notes that drive the smell detectors below.
|
||||
## Scope
|
||||
|
||||
## Why Test Smells Matter
|
||||
- Audit only staged or named tests. Search the current workspace before asking
|
||||
for code; never claim a file is missing until that search finds no relevant
|
||||
test.
|
||||
- Read production code only when it changes a verdict.
|
||||
- For unfamiliar framework APIs, call `test-analysis-extensions` and read the
|
||||
matching language extension.
|
||||
- Read [the complete catalog](references/test-smell-catalog.md) when the caller
|
||||
requests all 19 smells, asks for citations, or the code may contain a smell
|
||||
outside the high-signal set below. Do not load it for a narrow question that
|
||||
this file answers.
|
||||
|
||||
Test smells erode confidence in a test suite and inflate maintenance costs:
|
||||
## Audit Workflow
|
||||
|
||||
| Problem | Consequence |
|
||||
|---------|-------------|
|
||||
| Tests with conditional logic | Some paths never execute — hidden testing gaps |
|
||||
| Tests that depend on external resources | Flaky failures, slow execution, environment coupling |
|
||||
| Tests that sleep to wait for results | Non-deterministic timing, slow suites, false failures |
|
||||
| Tests without assertions | False confidence — coverage looks good but nothing is verified |
|
||||
| Tests that call many production methods | Hard to diagnose failures, unclear what's being tested |
|
||||
| Tests with magic numbers | Unreadable intent, unclear boundary conditions |
|
||||
| Tests relying on ToString for comparison | Brittle to formatting changes, obscure failure messages |
|
||||
| Tests with exception handling logic | Swallowed failures, tests that pass when they shouldn't |
|
||||
1. Search for and read the staged tests; detect language, framework, boundaries,
|
||||
and integration markers. This is the first action even when no path is named.
|
||||
2. Read the tests and only verdict-changing production context.
|
||||
3. For each candidate, verify the executed check, choose the formal category,
|
||||
calibrate, then assign severity. Report proven non-catalog test-validity
|
||||
defects separately; do not relabel them as smells.
|
||||
4. Rank confirmed findings by risk of false confidence or flakiness, then by
|
||||
maintenance cost.
|
||||
5. Give a framework-correct replacement for each actionable finding. Never use
|
||||
.NET terminology or APIs in another ecosystem.
|
||||
|
||||
## When to Use
|
||||
## High-Signal Decisions
|
||||
|
||||
- User asks for a comprehensive or formal test smell audit
|
||||
- User asks "are my tests well-written?" and wants a thorough analysis
|
||||
- User wants a test quality health check with academic rigor
|
||||
- User asks for a review of test design or structure using standard smell categories
|
||||
- User suspects tests are fragile, flaky, or giving false confidence and wants a deep investigation
|
||||
| Evidence | Academic finding | Do | Never |
|
||||
|---|---|---|---|
|
||||
| Assertion behavior changes behind `if`, `switch`, or branching loops | Conditional Test Logic | Split cases or parameterize them | Flag table-driven or parametrized tests merely because a runner loop exists |
|
||||
| A test relies on an undeclared file, network service, environment value, or database | Mystery Guest or Resource Optimism | Make the dependency explicit and hermetic; distinguish the two using the full catalog | Condemn an integration test merely for exercising its declared real resource |
|
||||
| Fixed wall-clock sleep waits for an outcome | Sleepy Test | Await or poll the condition with a timeout | Downgrade it only because the test is an integration test |
|
||||
| Executable test has no assertion, expected-exception marker, or mock verification | Unknown Test | Assert the observable outcome | Call an empty body Unknown Test; the formal name is Empty Test |
|
||||
| Async assertion/coroutine is created but not awaited or returned | Critical non-catalog false-pass defect | Report it separately and show the required `await`/`return` | Force it into Unknown Test; the assertion statement exists |
|
||||
| One test exercises many unrelated production behaviors | Eager Test | Separate behavior-focused tests | Flag a deliberate end-to-end workflow without considering its scope |
|
||||
| Expected numeric literal has no local meaning | Magic Number Test | Name the domain value or derive it from setup | Flag `count == 3` immediately after adding three items |
|
||||
| Assertion depends on `ToString`, `repr`, `description`, or display formatting that is not the contract | Sensitive Equality | Assert stable fields or use a structural matcher | Flag a test whose explicit contract is the formatted string |
|
||||
| Test manually manages expected exception flow | Exception Handling | Use the framework's exception assertion and check meaningful details | Claim a capture-and-assert test verifies nothing |
|
||||
| Shared setup creates state irrelevant to the tests that receive it | General Fixture | Remove unused state or narrow the fixture; rank cheap state low | Condemn relevant shared setup merely because it is shared |
|
||||
| Test is disabled or skipped | Ignored Test | Report every skip, but rank a tracked, reasoned skip below an unexplained one | Clear a skip because its reason is good, or give both the same urgency |
|
||||
|
||||
## When Not to Use
|
||||
## Calibration Rules
|
||||
|
||||
- User wants a quick pragmatic test review (use `test-anti-patterns` — faster, covers the most common issues)
|
||||
- User wants to evaluate assertion diversity specifically (use `assertion-quality`)
|
||||
- User wants to find duplicated boilerplate across tests (use `exp-test-maintainability`)
|
||||
- User wants to write new tests from scratch (help them directly)
|
||||
- User wants to fix a specific failing test (diagnose and fix directly)
|
||||
Apply these before assigning a finding:
|
||||
|
||||
## Inputs
|
||||
- Mock-call verifications, snapshots, bare pytest `assert`, Pester
|
||||
`Should -Invoke`, and expected-exception constructs are assertions.
|
||||
- A literal or snapshot assertion may expose a coverage gap, but is not Unknown
|
||||
Test or another smell without separate evidence.
|
||||
- Count assertion statements. One assertion is never Assertion Roulette;
|
||||
missing messages alone are not a smell.
|
||||
- Same-method tests are not Lazy Test when they cover distinct behaviors,
|
||||
boundaries, or state; require redundant equivalent paths.
|
||||
- General Fixture requires shared lifecycle state. Repeated local construction
|
||||
is neither General Fixture nor Test Code Duplication by itself.
|
||||
- Treat strings returned by the public API as observable contract unless
|
||||
production context or requirements make them display-only; interpolation
|
||||
alone is not Sensitive Equality.
|
||||
- Magic Number Test requires an unexplained oracle value. Do not flag ordinary
|
||||
setup quantities whose role is locally obvious and irrelevant to the asserted
|
||||
behavior.
|
||||
- Go table-driven subtests, pytest/JUnit/xUnit parameterization, Jest/Vitest
|
||||
`.each`, RSpec data tables, Pester `-ForEach`, and Catch2
|
||||
`SECTION`/`GENERATE` are not Conditional Test Logic by themselves.
|
||||
- Go's `if err != nil { t.Fatal(...) }` is idiomatic assertion flow, not
|
||||
Exception Handling.
|
||||
- Integration markers legitimize declared external resources and multi-step
|
||||
flows, but not fixed sleeps or assertion-free execution.
|
||||
- A local temporary file still meets the formal Mystery Guest definition.
|
||||
Hermetic creation and cleanup reduce its severity; they do not change its
|
||||
taxonomy.
|
||||
- A formatting name does not prove display text is the stable contract; confirm
|
||||
it from production behavior or requirements before clearing Sensitive
|
||||
Equality.
|
||||
- Do not infer a smell from method names alone. Point to the statement or
|
||||
fixture relationship that proves it.
|
||||
- If no material smell remains after calibration, say that clearly. Never
|
||||
manufacture findings to fill a report.
|
||||
- Never propose `await` for a void or otherwise non-awaitable API. If production
|
||||
work is synchronous, remove the sleep and assert immediately.
|
||||
|
||||
| Input | Required | Description |
|
||||
|-------|----------|-------------|
|
||||
| Test code | Yes | One or more test files or a test project directory to analyze |
|
||||
| Production code | No | The code under test, for context on whether patterns are justified |
|
||||
## Severity
|
||||
|
||||
## Workflow
|
||||
Severity follows demonstrated risk, not a fixed label copied from the catalog:
|
||||
|
||||
### Step 1: Detect language and load extension
|
||||
- **High:** can silently pass while behavior is broken, creates
|
||||
nondeterministic failures, or hides unexecuted assertion paths.
|
||||
- **Medium:** makes failures ambiguous or couples tests to unstable details.
|
||||
- **Low:** primarily maintenance debt, such as a reasoned skip or over-broad
|
||||
cheap fixture.
|
||||
|
||||
Identify the target codebase's language and test framework. Call the `test-analysis-extensions` skill and read the matching extension file (e.g., `extensions/dotnet.md`, `extensions/python.md`, `extensions/typescript.md`, `extensions/go.md`). The extension file lists the framework-specific test markers, sleep / wait APIs, skip / ignore attributes, mystery-guest indicators, and integration-test markers that the smell detectors below need.
|
||||
State the reason for the assigned severity. Downgrade or omit a finding when
|
||||
the surrounding test type makes the pattern intentional.
|
||||
|
||||
### Step 2: Gather the test code
|
||||
## Output Contract
|
||||
|
||||
Read all test files the user provides. If the user points to a directory or project, scan for all test files using the markers in the loaded language extension file.
|
||||
Scale the response to the input:
|
||||
|
||||
For a thorough audit, also consult the [extended smell catalog](references/test-smell-catalog.md) which covers 9 additional smell types beyond the core 10 below.
|
||||
- For one to three files, give a verdict and one compact table: severity,
|
||||
formal smell, evidence, risk, and fix.
|
||||
- For larger suites, add counts and a short priority order. Do not repeat
|
||||
findings across dashboards, prose, and plans.
|
||||
- Show code only when it clarifies a fix; omit unchanged setup.
|
||||
- Add brief **Not findings** only for plausibly suspicious idioms.
|
||||
- Do not narrate discovery or catalog loading; return the audit directly.
|
||||
|
||||
### Step 3: Scan for test smells
|
||||
|
||||
For each test method and class, check for the following smell categories. Examples reference .NET attributes but the patterns apply across all supported languages — use the loaded language extension file to map each pattern to the framework you are auditing.
|
||||
|
||||
#### Smell 1: Conditional Test Logic
|
||||
|
||||
Test methods containing `if`, `else`, `switch`, ternary (`? :`), `for`, `foreach`, `while`, or pattern-match arms that change assertion behavior. Control flow in tests means some paths may never execute, hiding gaps.
|
||||
|
||||
**Severity:** High
|
||||
**Detection:** Any control-flow statement inside a test method body that affects which assertions run.
|
||||
**Exceptions (per-language idioms, do NOT flag):**
|
||||
- **Foreach-assert** used solely to assert every item in a known collection (the assertion *is* the loop body).
|
||||
- **Go / Rust table-driven tests**: `for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... }) }` (Go) or `#[rstest]` parametrized loops are idiomatic.
|
||||
- **`it.each(...)` / `test.each(...)` / `@pytest.mark.parametrize` / `[Theory] + [InlineData]` / `@ParameterizedTest`** parametrization driven by data tables.
|
||||
- **Pester `-ForEach` / `-TestCases`** and **RSpec `where` blocks**.
|
||||
- **Catch2 `SECTION`s and `GENERATE(...)`**, **doctest `SUBCASE`**, **GoogleTest `INSTANTIATE_TEST_SUITE_P`**.
|
||||
|
||||
#### Smell 2: Mystery Guest
|
||||
|
||||
Tests that depend on external resources — files on disk, databases, network endpoints, environment variables — without making the dependency explicit or using test doubles.
|
||||
|
||||
**Severity:** High
|
||||
**Detection:** Test methods that read files, open database connections, make HTTP requests (without a test handler), read environment variables, or use hard-coded file paths. Per language: `File.ReadAllText` / `Directory.GetFiles` / `HttpClient` / `Environment.GetEnvironmentVariable` (.NET); `open()` / `pathlib.Path.read_text()` / `requests.get()` / `os.environ[...]` (Python); `fs.readFileSync` / `fetch(...)` / `process.env.X` (JS/TS); `Files.readAllBytes` / `Files.newInputStream` / `HttpClient.send` / `System.getenv` (Java); `os.ReadFile` / `http.Get` / `os.Getenv` (Go); `File.read` / `Net::HTTP.get` / `ENV[...]` (Ruby); `std::fs::read_to_string` / `reqwest::get` / `std::env::var` (Rust); `String(contentsOfFile:)` / `URLSession.shared.data` / `ProcessInfo.processInfo.environment` (Swift); `File(...).readText()` / `URL(...).openConnection()` / `System.getenv` (Kotlin); `Get-Content` / `Invoke-WebRequest` / `$env:X` (Pester); `std::ifstream` / `curl_easy_perform` / `std::getenv` (C++).
|
||||
**Exception:** In-memory fakes, test-specific handlers, or hermetic test data factories are fine.
|
||||
|
||||
#### Smell 3: Sleepy Test
|
||||
|
||||
Tests that call sleep or delay functions to wait for a condition. These introduce non-deterministic timing and slow down the suite.
|
||||
|
||||
**Severity:** High
|
||||
**Calibration:** Severity does **not** drop because the test is an integration test — a fixed sleep is still flaky and slow there. Report it as High and recommend polling/awaiting the condition with a timeout.
|
||||
**Detection:** Calls to sleep/delay functions inside test methods: `Thread.Sleep` / `Task.Delay` (.NET); `time.sleep` / `asyncio.sleep` (Python); `setTimeout` / `await new Promise(r => setTimeout(...))` / `jest.advanceTimersByTime` not paired with a wait (JS/TS); `Thread.sleep` / `TimeUnit.SECONDS.sleep` (Java); `time.Sleep` (Go); `sleep` / `Kernel#sleep` (Ruby); `std::thread::sleep` / `tokio::time::sleep` (Rust); `Thread.sleep` / `delay` (Kotlin coroutines); `sleep(_:)` / `Task.sleep` (Swift); `Start-Sleep` (Pester); `std::this_thread::sleep_for` (C++). See the matching language extension file for the full list.
|
||||
|
||||
#### Smell 4: Assertion-Free Test (Unknown Test)
|
||||
|
||||
Tests that execute code but never assert anything. Test frameworks report these as passing even if the code is completely broken, as long as no exception is thrown.
|
||||
|
||||
**Severity:** High
|
||||
**Detection:** A test method with no assertion calls and no expected-exception annotation. Framework-specific: missing `Assert.*` (.NET); no `assert` / `pytest.raises` (Python); no `expect(...)` or `assert.*` (JS/TS); no `assert*` / `assertThat` (Java); no `t.Error*` / `t.Fatal*` / `assert.*` testify (Go); no `expect`/`.to`/`.eq` (RSpec) or `assert*`/`refute*` (Minitest); no `assert*!` / `assert_eq!` / `panic!` (Rust); no `XCTAssert*` / `#expect` (Swift); no `assert*` / `should*` / Kotest matchers (Kotlin); no `Should -*` (Pester); no `EXPECT_*` / `ASSERT_*` / `REQUIRE` / `CHECK` (C++).
|
||||
**Calibration:**
|
||||
- A method named `*_DoesNotThrow` / `*_no_exception` / `should not throw` is implicitly asserting no exception — still flag it but note it may be intentional.
|
||||
- **Mock-call verifications count as assertions**: `mock.Verify(...)` (Moq), `Mock.AssertWasCalled` (NSubstitute), `mock.assert_called_with(...)` (Python), `expect(mock).toHaveBeenCalledWith(...)` (Jest), `verify(mock).method(...)` (Mockito), `Should -Invoke` (Pester) — do NOT flag tests using these as assertion-free.
|
||||
- **Bare assertion forms count**: `assert x == y` (pytest), `if got != want { t.Errorf(...) }` (Go), `assert!(cond)` (Rust) are canonical.
|
||||
- **Snapshot assertions count**: `.toMatchSnapshot()` (Jest), `syrupy` (pytest), `SnapshotTesting` (Swift), `approval-tests` are real assertions.
|
||||
- **Missing await on async assertions is its own critical smell**: `expect(promise).resolves.toBe(x)` without `await`/`return` (Jest), un-awaited `Assert.ThrowsAsync` (xUnit), un-awaited coroutines in `pytest-asyncio`, Kotest tests without `runTest`, Swift Testing async cases without `await`. These tests have assertion calls but silently pass — flag with a dedicated note.
|
||||
|
||||
#### Smell 5: Eager Test
|
||||
|
||||
A test method that calls many different production methods, making it unclear what behavior is being tested. When it fails, diagnosis is difficult because the failure could stem from any of the calls.
|
||||
|
||||
**Severity:** Medium
|
||||
**Detection:** A test method that calls 4+ distinct methods on the production object (excluding setup/construction). Count unique method names, not call count.
|
||||
**Calibration:** Integration / end-to-end / workflow tests may legitimately call multiple methods. Check for integration markers in the loaded language extension file (e.g., `[Trait("Category", "Integration")]`, `@Tag("integration")`, `pytest.mark.integration`, `*_integration_test.go`, `Describe ... -Tag 'Integration'`) and downgrade.
|
||||
|
||||
#### Smell 6: Magic Number Test
|
||||
|
||||
Assertions that contain unexplained numeric literals. The intent of `Assert.AreEqual(42, result)` / `assert result == 42` / `expect(result).toBe(42)` is unclear without context — what does 42 represent?
|
||||
|
||||
**Severity:** Medium
|
||||
**Detection:** Numeric literals (other than 0, 1, -1, and the literal used in the test name) appearing as `expected` parameters in assertion methods or comparison operands.
|
||||
**Calibration:** Small integers in context (like count checks `Assert.AreEqual(3, list.Count)` / `assert len(items) == 3` / `expect(arr.length).toBe(3)` where 3 items were just added) are acceptable — only flag when the number's meaning is genuinely unclear.
|
||||
|
||||
#### Smell 7: Sensitive Equality
|
||||
|
||||
Tests that use string conversion for comparison or assertion. If the underlying string representation changes, the test breaks even though the actual behavior is correct.
|
||||
|
||||
**Severity:** Medium
|
||||
**Detection:** `Assert.AreEqual(expected, obj.ToString())` (.NET); `assert str(obj) == "..."` or `assert repr(obj) == "..."` (Python); `expect(obj.toString()).toBe("...")` or `expect(`${obj}`).toBe(...)` (JS/TS); `assertEquals(expected, obj.toString())` (Java); `assert.Equal(t, "...", fmt.Sprint(obj))` or `obj.String()` chains (Go); `expect(obj.to_s).to eq("...")` (RSpec); `assert_eq!(format!("{}", obj), "...")` or `assert_eq!(format!("{:?}", obj), "...")` (Rust); `XCTAssertEqual(obj.description, "...")` or string-interpolation assertion (Swift); `assertEquals("...", obj.toString())` (Kotlin); `Should -Be "..."` against a `[string]$obj` (Pester); `EXPECT_EQ("...", std::to_string(obj))` (C++).
|
||||
|
||||
#### Smell 8: Exception Handling in Tests
|
||||
|
||||
Tests that contain `try`/`catch`/`except`/`rescue` blocks or `throw`/`raise`/`panic`/`return err` statements used to manage exception flow instead of asserting on it. This typically means the test is manually managing errors rather than using the framework's built-in exception assertion facilities.
|
||||
|
||||
**Severity:** Medium
|
||||
**Detection:** `try`/`catch` (.NET, Java, JS/TS, Kotlin, Swift, C++); `try`/`except` (Python); `begin`/`rescue` (Ruby); `defer recover()` (Go); manual `if err != nil { t.Fatal(err) }` in Go is canonical and NOT a smell.
|
||||
**Exception:** `catch`/`except`/`rescue` blocks that capture an exception for further assertion on its properties are a lesser concern — note but don't flag as high severity.
|
||||
|
||||
#### Smell 9: General Fixture (Over-broad Setup)
|
||||
|
||||
The test setup method, constructor, or fixture initializes fields that are not used by every test method. This means each test pays the cost of setting up objects it doesn't need.
|
||||
|
||||
**Severity:** Low
|
||||
**Detection:** Fields/properties initialized in `[TestInitialize]` / `setUp` / `@BeforeEach` / `beforeEach` / `before(:each)` / `BeforeEach` (Pester) / `setUpWithError` (XCTest) / pytest `fixture(autouse=True)` / xUnit constructor / Kotest `beforeTest` that are referenced by fewer than half the test methods in the class/module/file.
|
||||
|
||||
#### Smell 10: Ignored / Disabled / Skipped Test
|
||||
|
||||
Tests marked as skipped or disabled. These add overhead and clutter, and the underlying issue they were disabled for may never be addressed.
|
||||
|
||||
**Severity:** Low
|
||||
**Detection:** Skip / ignore / disable annotations or conditional compilation that disables a test. See the loaded language extension file for framework-specific skip attributes — e.g., `[Ignore]` (MSTest/NUnit), `Skip = "..."` (xUnit `Fact`), `@Ignore` (TUnit/JUnit 4), `@Disabled` (JUnit 5), `@pytest.mark.skip` / `pytest.skip(...)` / `pytestmark`, `it.skip` / `xit` / `describe.skip` / `test.skip` (Jest/Vitest/Mocha), `t.Skip(...)` (Go), `pending` / `skip` / `xit` (RSpec), `#[ignore]` (Rust), `XCTSkip` / `@Test(.disabled)` (Swift), `@Ignored` (Kotest), `-Skip` (Pester), `GTEST_SKIP()` / `DISABLED_TestName` (GoogleTest), `[.]` tag (Catch2), `TEST_CASE("...", "[.]")` skip.
|
||||
|
||||
### Step 4: Apply calibration rules
|
||||
|
||||
Before reporting, calibrate findings to avoid false positives:
|
||||
|
||||
- **Integration tests have different norms — but not for sleeps.** A test class clearly marked as integration (by name, annotation, category, or convention — see the loaded language extension file for markers) legitimately uses external resources and calls multiple methods. Downgrade Mystery Guest and Eager Test for integration tests. **Do NOT downgrade Sleepy Test:** a fixed wall-clock sleep is non-deterministic and slow in any test category, so it stays a real High-severity smell — recommend polling/awaiting the condition with a timeout instead. Only treat a sleep as acceptable when it is bounded by a documented external constraint (e.g. a third-party rate limit) *and* paired with a condition check.
|
||||
- **Simple loop-assert patterns are fine.** Iterating a collection to assert on every item is readable and correct. Only flag loops with complex branching logic.
|
||||
- **Idiomatic table-driven and parametrized patterns are NOT Conditional Test Logic.** Go's `for _, tt := range tests { t.Run(...) }`, Rust's `#[rstest]`, pytest's `@parametrize`, Jest/Vitest `.each`, JUnit `@ParameterizedTest`, RSpec `where`, Pester `-ForEach`, Catch2 `SECTION`/`GENERATE`, GoogleTest `INSTANTIATE_TEST_SUITE_P` are canonical and must NOT be flagged.
|
||||
- **Context matters for magic numbers.** A count assertion right after adding a known number of items is self-documenting. Only flag numbers whose meaning requires looking at production code to understand.
|
||||
- **Bare `assert` (pytest) is canonical, not assertion-free framework use.** Don't flag.
|
||||
- **Go's `if err != nil { t.Fatal(err) }` is canonical**, not Exception Handling in Tests. Don't flag.
|
||||
- **Mock-call verifications and snapshot assertions are real assertions** — do not flag tests using them as Assertion-Free.
|
||||
- **Missing-await on async assertions is its own critical sub-smell of Assertion-Free** — these tests silently pass even when the underlying assertion fails. Always flag when detected.
|
||||
- **Inconclusive/pending markers are not assertion-free.** Tests explicitly marked as incomplete should be flagged as Ignored Test, not Assertion-Free.
|
||||
- **Capture-and-assert exception patterns are borderline.** `try { ... } catch (X x) { Assert.Equal(...) }` style patterns are ugly but functional. Note as a smell and suggest the framework's built-in exception assertion (`Assert.Throws<T>`, `pytest.raises`, `expect(fn).toThrow`, `assertThrows`, `assert.PanicsWithError`, etc.) instead of calling it broken.
|
||||
- **If the test suite is clean, say so.** A report finding few or no smells is perfectly valid.
|
||||
|
||||
### Step 5: Report findings
|
||||
|
||||
Present the analysis in this structure:
|
||||
|
||||
1. **Summary Dashboard** — Quick overview:
|
||||
```
|
||||
| Severity | Smell Count | Affected Tests |
|
||||
|----------|-------------|----------------|
|
||||
| High | 3 | 7 |
|
||||
| Medium | 2 | 4 |
|
||||
| Low | 1 | 2 |
|
||||
| Total | 6 | 13 |
|
||||
```
|
||||
|
||||
2. **Findings by Severity** — For each smell found:
|
||||
- Smell name and category
|
||||
- Severity level with rationale
|
||||
- Affected test methods (file and method name)
|
||||
- Code snippet showing the smell
|
||||
- Concrete fix: show what the code should look like after remediation
|
||||
- Risk if left unfixed
|
||||
|
||||
3. **Smell-Free Patterns** — If any test methods are well-written, briefly acknowledge this. Highlighting what's good helps the user understand the contrast.
|
||||
|
||||
4. **Prioritized Remediation Plan** — Rank fixes by:
|
||||
- Impact (high-severity smells affecting many tests first)
|
||||
- Effort (quick fixes before refactoring)
|
||||
- Risk (fixes that prevent false-passes before cosmetic improvements)
|
||||
Every reported smell must have a formal taxonomy name, precise location,
|
||||
evidence from the code, practical risk, and a concrete framework-correct fix.
|
||||
|
||||
## Validation
|
||||
|
||||
- [ ] Every finding includes the specific test method name and file location
|
||||
- [ ] Every finding includes a code snippet showing the smell in context
|
||||
- [ ] Every finding includes a concrete fix example (not just "fix this")
|
||||
- [ ] Integration tests are not penalized for using real resources, but their fixed sleeps are still reported as High
|
||||
- [ ] Each smell is reported under its own taxonomy name (Unknown Test, Empty Test, Assertion Roulette are distinct — do not merge them)
|
||||
- [ ] Simple foreach-assert loops are not flagged as conditional test logic
|
||||
- [ ] Contextually obvious numbers are not flagged as magic numbers
|
||||
- [ ] If the test suite is clean, the report says so upfront
|
||||
- [ ] Severity levels are justified, not arbitrary
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
| Pitfall | Solution |
|
||||
|---------|----------|
|
||||
| Flagging integration tests for using real resources | Check for integration test markers (per the loaded language extension) and adjust severity accordingly — external resources and multi-step flows are expected there |
|
||||
| Calibrating away a real sleep as an "integration style issue" | `Thread.Sleep(3000)` in an integration test is still a High-severity Sleepy Test; recommend a polled wait with timeout |
|
||||
| Flagging loop-over-collection-assert as conditional logic | Only flag loops with branching or complex logic, not assertion iterations |
|
||||
| Flagging Go/Rust table-driven loops as Conditional Test Logic | `for _, tt := range tests { t.Run(...) }` (Go) and `#[rstest]` loops (Rust) are canonical and must NOT be flagged |
|
||||
| Flagging parametrized tests as Duplicate Assert | `@pytest.mark.parametrize`, `it.each`, `[Theory]+[InlineData]`, `@ParameterizedTest`, RSpec `where`, Pester `-ForEach`, Catch2 `SECTION`/`GENERATE` are correct deduplication, not smells |
|
||||
| Flagging obvious count assertions after adding N items | Consider the immediate context — self-documenting numbers are fine |
|
||||
| Missing framework-specific assertion syntax | Always read the matching language extension file first; each framework has distinct assertion APIs (xUnit `Assert.Equal`, MSTest `Assert.AreEqual`, NUnit `Is.EqualTo`, pytest bare `assert`, Jest `expect().toBe()`, etc.) |
|
||||
| Treating mock-call verifications as assertion-free | `mock.Verify(...)`, `expect(mock).toHaveBeenCalledWith(...)`, `Should -Invoke`, `verify(mock).method(...)`, `mock.assert_called_with(...)` are real assertions |
|
||||
| Missing the async-test silent-pass trap | Always flag `expect(promise).resolves.toBe(x)` without `await`/`return`, un-awaited `Assert.ThrowsAsync` (xUnit), un-awaited coroutines in pytest-asyncio, missing `runTest` in Kotest, un-awaited Swift Testing async assertions |
|
||||
| Over-flagging try/catch that captures for assertion | Distinguish swallowed exceptions from capture-and-assert patterns |
|
||||
| Treating skip annotations with reasons same as bare skips | Note that reasoned skips (`Skip = "Tracked by #123"`, `@pytest.mark.skip(reason="...")`, `t.Skip("not yet implemented")`) are less concerning than unexplained ones |
|
||||
| Flagging `DoesNotThrow`-style tests as assertion-free | These implicitly assert no exception — note but acknowledge the intent |
|
||||
- Every finding is supported by code, not a keyword or method name.
|
||||
- Unknown Test and Empty Test remain distinct.
|
||||
- Every disabled test remains Ignored Test, and every local file dependency
|
||||
remains Mystery Guest; rationale and hermetic cleanup change severity only.
|
||||
- Framework idioms and integration boundaries were calibrated before reporting.
|
||||
- Clean tests and suspicious-but-valid idioms are not turned into filler.
|
||||
- Fixes use the target framework's APIs and preserve the behavior under test.
|
||||
- Claims about files reviewed, builds, or test runs match actions actually
|
||||
performed.
|
||||
|
||||
@@ -8,14 +8,14 @@ Source: [testsmells.org](https://testsmells.org/) — a research project from th
|
||||
|
||||
The academic literature identifies 19 distinct test smell types. The core skill covers the 10 most impactful ones. This catalog documents all 19 for deeper analysis when requested.
|
||||
|
||||
### Smells Covered by the Core Skill
|
||||
### High-Signal Smells Summarized by the Core Skill
|
||||
|
||||
| Smell | Core Skill | Academic Name |
|
||||
| ----- | ---------- | ------------- |
|
||||
| Conditional Test Logic | Smell 1 | Conditional Test Logic |
|
||||
| Mystery Guest | Smell 2 | Mystery Guest |
|
||||
| Sleepy Test | Smell 3 | Sleepy Test |
|
||||
| Assertion-Free Test | Smell 4 | Unknown Test / Empty Test |
|
||||
| Assertion-Free Test | Smell 4 | Unknown Test |
|
||||
| Eager Test | Smell 5 | Eager Test |
|
||||
| Magic Number Test | Smell 6 | Magic Number Test |
|
||||
| Sensitive Equality | Smell 7 | Sensitive Equality |
|
||||
@@ -25,13 +25,20 @@ The academic literature identifies 19 distinct test smell types. The core skill
|
||||
|
||||
### Additional Smells (Extended Analysis)
|
||||
|
||||
These smells are not in the core skill but can be reported when the user requests a thorough audit or when they are particularly prevalent.
|
||||
These smells are not summarized in the core skill but remain part of the
|
||||
19-smell taxonomy. Report them when the user requests a complete audit or the
|
||||
code provides evidence for one of these categories.
|
||||
|
||||
#### Assertion Roulette
|
||||
|
||||
A test method has multiple assertions without descriptive messages. When one fails, it's unclear which assertion caused the failure and why.
|
||||
|
||||
**Detection:** A test method containing 3+ assertion statements where none provide an explanation message parameter.
|
||||
**Detection:** A test method containing more than one assertion statement where
|
||||
none provides an explanation message parameter.
|
||||
|
||||
Count assertion statements, not values compared inside a collection or
|
||||
structural matcher. A single assertion cannot be Assertion Roulette, and a
|
||||
missing message on one assertion is not sufficient evidence.
|
||||
|
||||
**Example:**
|
||||
|
||||
@@ -66,6 +73,10 @@ Multiple test methods test the same production method. While not always a proble
|
||||
|
||||
**Detection:** Multiple test methods in the same class calling the same production method as their primary action.
|
||||
|
||||
Do not report distinct behaviors, boundary cases, or state transitions merely
|
||||
because they call the same method. Confirm that the paths are equivalent and
|
||||
redundant before assigning this smell.
|
||||
|
||||
#### Constructor Initialization
|
||||
|
||||
Test class uses a constructor instead of the framework's setup method to initialize fields. This bypasses framework lifecycle hooks and can cause issues with test isolation.
|
||||
@@ -104,7 +115,9 @@ Tests that assume external resources (files, services) exist without checking. T
|
||||
|
||||
#### Empty Test
|
||||
|
||||
A test method that contains no executable statements — only comments or whitespace. Similar to Assertion-Free Test but even more extreme: no code runs at all.
|
||||
A test method that contains no executable statements — only comments or
|
||||
whitespace. Keep this distinct from Unknown Test, which executes production
|
||||
code but contains no assertion.
|
||||
|
||||
**Detection:** Test method body contains only comments, whitespace, or commented-out code.
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ description: Evaluates the dotnet-test/test-smell-detection skill
|
||||
type: capability
|
||||
defaults:
|
||||
timeout: 5m
|
||||
runs: 2
|
||||
runs: 1
|
||||
stimuli:
|
||||
- name: Detect multiple test smells in order processing test suite
|
||||
prompt: |
|
||||
@@ -18,30 +18,24 @@ stimuli:
|
||||
dest: OrderService.Tests/OrderService.Tests.csproj
|
||||
- src: fixtures/smelly-tests/OrderService.Tests/OrderProcessorTests.cs
|
||||
dest: OrderService.Tests/OrderProcessorTests.cs
|
||||
- src: fixtures/smelly-tests/OrderService.Tests/OrderServiceSupport.cs
|
||||
dest: OrderService.Tests/OrderServiceSupport.cs
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (conditional|if.else|control flow|branch)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (no assert|assertion.free|without.*assert|zero assert|missing.*assert)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Thread\.Sleep|sleep|delay|sleepy)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Identified the conditional test logic in ProcessOrder_SetsCorrectStatus — the if/else means one branch is always
|
||||
untested
|
||||
- Flagged ProcessOrder_CompletesWithoutError as having no assertions — it only calls the method without verifying
|
||||
anything
|
||||
- Identified the Thread.Sleep(2000) in ProcessOrder_AsyncNotification_IsSent as a flakiness risk
|
||||
- Noted the try/catch pattern in ProcessOrder_EmptyOrder_ThrowsValidationError and suggested using
|
||||
Assert.ThrowsException instead
|
||||
- Identified the ToString() comparison in GetOrderSummary_ReturnsFormattedString as fragile
|
||||
- Noted the File.ReadAllText with a hard-coded path in ImportOrders_FromCsv_ParsesCorrectly as an external
|
||||
dependency problem
|
||||
- Provided concrete fix suggestions showing how to rewrite at least some of the smelly tests
|
||||
- Explained how the output-dependent branch in ProcessOrder_SetsCorrectStatus leaves behavior untested and applied
|
||||
the appropriate formal category
|
||||
- Explained that ProcessOrder_CompletesWithoutError can pass without observing a result and applied the appropriate
|
||||
formal category
|
||||
- Connected the fixed wait in ProcessOrder_AsyncNotification_IsSent to latency or flakiness
|
||||
- Replaced the manual exception-flow pattern with a framework-native exception assertion
|
||||
- Identified the display-string comparison in GetOrderSummary_ReturnsOrderDetails as coupling the test to unstable
|
||||
representation instead of stable OrderSummary fields
|
||||
- Explained why the hard-coded CSV path is an undeclared and optimistic external dependency
|
||||
- Noted that shared setup creates an unused logger and calibrated this maintenance-only fixture issue below
|
||||
false-pass and reliability risks
|
||||
- Proposed concrete, behavior-preserving rewrites for the most important findings
|
||||
- name: Recognize well-written tests with no significant smells
|
||||
prompt: |
|
||||
Review my Calculator tests against the academic test-smell catalog. I want
|
||||
@@ -54,16 +48,18 @@ stimuli:
|
||||
dest: Calculator.Tests/Calculator.Tests.csproj
|
||||
- src: fixtures/clean-tests/Calculator.Tests/ScientificCalculatorTests.cs
|
||||
dest: Calculator.Tests/ScientificCalculatorTests.cs
|
||||
- src: fixtures/clean-tests/Calculator.Tests/ScientificCalculator.cs
|
||||
dest: Calculator.Tests/ScientificCalculator.cs
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (clean|good|well.written|no.*(major|significant).*(smell|issue|problem)|solid|well.structured)
|
||||
pattern: (?s)(clean|good|well.written|no.*(major|significant).*(smell|issue|problem)|solid|well.structured)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Recognized that the test suite is well-structured with clear Arrange-Act-Assert patterns
|
||||
- Acknowledged proper use of Assert.ThrowsException for exception testing instead of try/catch
|
||||
- Noted the use of parameterized tests with [DataTestMethod] as a good practice
|
||||
- Concluded that the test suite has no material academic smells
|
||||
- Treated the exception assertion as a real check rather than assertion-free execution
|
||||
- Treated the data-driven cases as intentional parameterization rather than conditional logic or duplication
|
||||
- Did not invent false problems or flag correct patterns as smells
|
||||
- If any minor suggestions were made, they were presented as optional improvements rather than problems
|
||||
- name: Recognize integration tests and avoid false positives for external resources
|
||||
@@ -79,24 +75,22 @@ stimuli:
|
||||
dest: DataAccess.IntegrationTests/DataAccess.IntegrationTests.csproj
|
||||
- src: fixtures/integration-tests/DataAccess.IntegrationTests/UserRepositoryIntegrationTests.cs
|
||||
dest: DataAccess.IntegrationTests/UserRepositoryIntegrationTests.cs
|
||||
- src: fixtures/integration-tests/DataAccess.IntegrationTests/RepositorySupport.cs
|
||||
dest: DataAccess.IntegrationTests/RepositorySupport.cs
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (integration|appropriate|expected|legitimate|acceptable)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Sleep|delay|sleepy|conditional|if.else|assert|no assert)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Recognized that these are integration tests and did not flag database usage or insert-then-query patterns as
|
||||
smells
|
||||
- Identified the Thread.Sleep(3000) in NotifyOnInsert_SendsEventAfterDelay as a real smell even in an integration
|
||||
test context
|
||||
- Flagged the if/else conditional logic in GetUser_ReturnsCorrectType as a test smell — one branch is untested
|
||||
- Identified BulkInsert_RunsWithoutErrors as having no assertions — it exercises code but verifies nothing
|
||||
- Explained why the fixed wait remains a reliability problem even in an integration test
|
||||
- Explained how the output-dependent branch in GetUser_ReturnsCorrectType weakens the oracle
|
||||
- Explained that BulkInsert_RunsWithoutErrors can pass without verifying persisted state
|
||||
- Distinguished between the legitimate integration patterns (setup/teardown, multi-step persistence) and the
|
||||
actual smells (sleep, conditional, assertion-free)
|
||||
actual risks, while applying appropriate formal taxonomy names
|
||||
|
||||
- name: Separate reasoned skips and self-documenting numbers from real smells
|
||||
prompt: |
|
||||
@@ -120,21 +114,12 @@ stimuli:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Ignored Test|skip|Skip)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Sleepy|Thread\.Sleep|sleep)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Assertion.?Free|no assert|without.*assert)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (High|Medium|Low|severity)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Flagged the Thread.Sleep(3000) in Replenish_AsyncJob_Completes as a High-severity Sleepy Test
|
||||
- Flagged Audit_RunsWithoutError as an Assertion-Free Test — it exercises the service but verifies nothing
|
||||
- Reported both skipped tests as Ignored Test, but treated the bare `Skip = "skip"` on
|
||||
- Ranked the fixed wait in Replenish_AsyncJob_Completes as a high-severity reliability risk
|
||||
- Explained that Audit_RunsWithoutError executes the service without an observable check
|
||||
- Applied the formal skipped-test category to both disabled tests, but treated the bare `Skip = "skip"` on
|
||||
Restock_FromSupplier_UpdatesQuantities as more concerning than the reasoned skip on
|
||||
Reserve_AcrossWarehouses_BalancesStock, which documents why and links a tracking issue
|
||||
- Did not flag the contextually obvious numbers as Magic Number Test — asserting a count of 3 right after
|
||||
@@ -142,11 +127,13 @@ stimuli:
|
||||
- Severity levels are justified with a stated rationale rather than assigned arbitrarily
|
||||
- Every finding includes a concrete fix example — the rewritten code — rather than a bare "fix this"
|
||||
instruction
|
||||
- Used xUnit terminology and APIs (Fact, Skip, Assert.Equal) rather than describing the suite in MSTest terms
|
||||
- Proposed fixes in xUnit terminology rather than translating APIs from another framework
|
||||
- name: Audit a JUnit suite using Java-specific smell markers
|
||||
prompt: >
|
||||
Review the tests in this Java project and tell me which test smells they
|
||||
have, with severity. I want to know what to fix first.
|
||||
Give this Java test file a formal, research-backed test-smell review.
|
||||
Use the recognized category names, rank genuine risks by severity, and
|
||||
keep the recommendations in JUnit terminology. I want to know what to fix
|
||||
first, not how many labels you can produce.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/junit-smells/pom.xml
|
||||
@@ -158,23 +145,127 @@ stimuli:
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (High|Medium|Low|severity)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (addAndDoNothing|assertion.free|no assertion|Unknown Test)
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (sleep|Sleepy)
|
||||
pattern: (addAndDoNothing|no assertion|without verifying)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Flagged addAndDoNothing as an assertion-free test that verifies nothing
|
||||
- Flagged the Thread.sleep in addSleepy as a Sleepy Test
|
||||
- Flagged the branching inside addConditionalLogic as Conditional Test Logic
|
||||
- Flagged addLoadsFromFile as a Mystery Guest for depending on the file system
|
||||
- Flagged doEverythingAtOnce as an Eager Test that exercises many distinct methods
|
||||
- Flagged the unexplained 42 in quantityAfterAdds as a Magic Number Test
|
||||
- Flagged toStringFormat as Sensitive Equality for depending on the toString format
|
||||
- Flagged addNullSkuManualCatch for hand-rolling try/catch instead of using assertThrows
|
||||
- Flagged the @Disabled test as an Ignored Test
|
||||
- Explained that addAndDoNothing can pass without observing behavior and assigned the appropriate formal category
|
||||
- Connected the fixed wait in addSleepy to avoidable latency or timing risk
|
||||
- Explained how branching inside addConditionalLogic hides per-case failures
|
||||
- Distinguished the temporary-file dependency and cleanup concern from ordinary in-memory setup
|
||||
- Explained why doEverythingAtOnce obscures several distinct behaviors behind one aggregate check
|
||||
- Did not call the locally derived 42 a magic number or call an explicit formatting contract fragile solely because
|
||||
it uses toString
|
||||
- Replaced broad manual exception handling with a precise JUnit exception assertion
|
||||
- Reported the disabled test and calibrated its urgency from the weak rationale
|
||||
- Noted that shared setup creates unused rng state without condemning the relevant catalog fixture
|
||||
- Recognised addThrowsOnNegativeCount as a well-written test rather than condemning the whole file
|
||||
- Used JUnit terminology and Java APIs throughout rather than describing the suite in MSTest terms
|
||||
- Used JUnit concepts and fixes throughout rather than translating APIs from another framework
|
||||
- name: Clear idiomatic pytest patterns without inventing smells
|
||||
prompt: |
|
||||
I need a formal test-smell assessment of `test_cart.py` for a Python
|
||||
review. Use recognized research taxonomy names if anything is genuinely
|
||||
wrong, but be explicit when suspicious-looking constructs are valid
|
||||
pytest idioms. Keep the answer concise.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/pytest-idioms/test_cart.py
|
||||
dest: test_cart.py
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (?s)(clean|no.*(material|significant|genuine).*smell|well.written|valid.*idiom)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Concluded that the file has no material academic test smells instead of manufacturing findings
|
||||
- Recognized parametrization as idiomatic data-driven testing rather than Conditional Test Logic or duplication
|
||||
- Counted the mock verification and exception context as real checks
|
||||
- Did not misclassify the simple loop whose body only asserts each item
|
||||
- Used pytest and Python terminology, with a concise verdict rather than a large empty dashboard
|
||||
- name: Catch an unawaited Jest async assertion without condemning snapshots
|
||||
prompt: |
|
||||
Formally review `user-service.test.ts` for research-named test smells.
|
||||
Explain whether either test can pass without checking what it appears to
|
||||
check, and show the smallest Jest-correct fix. Do not flag valid assertion
|
||||
forms merely because they are unfamiliar.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/jest-async/user-service.test.ts
|
||||
dest: user-service.test.ts
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (?s)(await|return).*expect|expect.*(await|return)|unawaited|not awaited
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Identified that the unresolved `resolves` chain is not awaited or returned, so the test can finish before the assertion
|
||||
- Kept the silent false-pass risk separate from the 19-smell taxonomy rather than mislabeling an existing assertion as Unknown Test
|
||||
- Showed the minimal Jest fix using `async` plus `await expect(loadUser()).resolves.toEqual(...)`, or returning the assertion promise
|
||||
- Recognized the snapshot matcher as a real assertion and did not force the literal subject into an unrelated
|
||||
academic category
|
||||
- Used Jest and TypeScript concepts rather than translating APIs from another framework
|
||||
- name: Distinguish a Go table-driven test from a fixed sleep
|
||||
prompt: |
|
||||
Audit `cache_test.go` with the formal test-smell taxonomy. Separate
|
||||
idiomatic Go test structure from behavior that creates a real reliability
|
||||
risk, justify severity, and give a Go-appropriate replacement strategy.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/go-table/cache_test.go
|
||||
dest: cache_test.go
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Sleepy Test|time\.Sleep|fixed sleep)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Flagged only the fixed `time.Sleep` synchronization as a high-risk Sleepy Test
|
||||
- Recommended an observable Go completion mechanism rather than translating a fix from another framework
|
||||
- Recognized the `for` plus `t.Run` structure as idiomatic table-driven testing, not Conditional Test Logic
|
||||
- Treated the ordinary error-reporting branch as canonical Go assertion flow rather than manual exception handling
|
||||
- Kept the response focused on the one real finding and the important non-findings
|
||||
- name: Count Pester mock verification as an assertion while flagging sleep
|
||||
prompt: |
|
||||
Apply the formal test-smell taxonomy to `Worker.Tests.ps1`. Tell me which
|
||||
construct is a genuine risk and which PowerShell testing construct already
|
||||
verifies behavior. Include a Pester-native fix direction.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/pester-mixed/Worker.Tests.ps1
|
||||
dest: Worker.Tests.ps1
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (Sleepy Test|Start-Sleep|fixed sleep)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Flagged `Start-Sleep` as the genuine Sleepy Test because fixed timing makes the suite slow and unreliable
|
||||
- Counted the Pester mock verification as a real behavioral check, so neither test was treated as assertion-free
|
||||
- Suggested a PowerShell-appropriate observable completion mechanism or bounded poll
|
||||
- Kept the proposed fix native to Pester and PowerShell
|
||||
- name: Recognize Catch2 sections and generators as clean parameterization
|
||||
prompt: |
|
||||
Is `parser_tests.cpp` clean under the formal academic test-smell
|
||||
taxonomy? Review the control-flow-looking constructs carefully and report
|
||||
only evidence-backed findings. This is a concise C++ test review.
|
||||
environment:
|
||||
files:
|
||||
- src: fixtures/catch2-idioms/parser_tests.cpp
|
||||
dest: parser_tests.cpp
|
||||
graders:
|
||||
- type: output-matches
|
||||
config:
|
||||
pattern: (?s)(clean|no.*(material|significant|genuine).*smell|idiomatic|valid)
|
||||
- type: exit-success
|
||||
- type: prompt
|
||||
rubric:
|
||||
- Concluded that the file has no material test smells
|
||||
- Recognized generator and section constructs as idiomatic parameterization and case structure rather than
|
||||
application-controlled branching
|
||||
- Recognized the Catch2 matcher as a real assertion
|
||||
- Did not invent smells to satisfy the request or produce an oversized empty report
|
||||
- Used Catch2 and C++ terminology throughout
|
||||
|
||||
@@ -0,0 +1,16 @@
|
||||
#include <catch2/catch_test_macros.hpp>
|
||||
#include <catch2/generators/catch_generators.hpp>
|
||||
|
||||
#include <string>
|
||||
|
||||
TEST_CASE("parser accepts supported separators")
|
||||
{
|
||||
const auto separator = GENERATE(',', ';');
|
||||
|
||||
SECTION("a generated separator is preserved")
|
||||
{
|
||||
const std::string input = std::string("left") + separator + "right";
|
||||
|
||||
REQUIRE(input.at(4) == separator);
|
||||
}
|
||||
}
|
||||
+40
@@ -0,0 +1,40 @@
|
||||
namespace Calculator.Tests;
|
||||
|
||||
public sealed class ScientificCalculator
|
||||
{
|
||||
private readonly List<string> _history = [];
|
||||
|
||||
public int Add(int left, int right)
|
||||
{
|
||||
_history.Add($"Add({left}, {right})");
|
||||
return left + right;
|
||||
}
|
||||
|
||||
public double Divide(double dividend, double divisor)
|
||||
{
|
||||
if (divisor == 0)
|
||||
{
|
||||
throw new DivideByZeroException();
|
||||
}
|
||||
|
||||
_history.Add($"Divide({dividend}, {divisor})");
|
||||
return dividend / divisor;
|
||||
}
|
||||
|
||||
public double SquareRoot(double value)
|
||||
{
|
||||
if (value < 0)
|
||||
{
|
||||
throw new ArgumentException("Value must be non-negative.", nameof(value));
|
||||
}
|
||||
|
||||
_history.Add($"SquareRoot({value})");
|
||||
return Math.Sqrt(value);
|
||||
}
|
||||
|
||||
public double Exp(double value) => Math.Exp(value);
|
||||
|
||||
public IReadOnlyList<string> GetHistory() => _history;
|
||||
|
||||
public void ClearHistory() => _history.Clear();
|
||||
}
|
||||
+6
-9
@@ -30,7 +30,7 @@ public sealed class ScientificCalculatorTests
|
||||
{
|
||||
var calc = new ScientificCalculator();
|
||||
|
||||
Assert.ThrowsException<DivideByZeroException>(
|
||||
Assert.ThrowsExactly<DivideByZeroException>(
|
||||
() => calc.Divide(10, 0));
|
||||
}
|
||||
|
||||
@@ -49,10 +49,8 @@ public sealed class ScientificCalculatorTests
|
||||
{
|
||||
var calc = new ScientificCalculator();
|
||||
|
||||
var ex = Assert.ThrowsException<ArgumentException>(
|
||||
Assert.ThrowsExactly<ArgumentException>(
|
||||
() => calc.SquareRoot(-1));
|
||||
|
||||
Assert.AreEqual("value", ex.ParamName);
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
@@ -75,7 +73,7 @@ public sealed class ScientificCalculatorTests
|
||||
Assert.AreEqual(5.0, result);
|
||||
}
|
||||
|
||||
[DataTestMethod]
|
||||
[TestMethod]
|
||||
[DataRow(0, 1.0)]
|
||||
[DataRow(1, 2.718281828)]
|
||||
[DataRow(-1, 0.367879441)]
|
||||
@@ -98,10 +96,9 @@ public sealed class ScientificCalculatorTests
|
||||
|
||||
var history = calc.GetHistory();
|
||||
|
||||
Assert.AreEqual(3, history.Count);
|
||||
Assert.IsTrue(history[0].Contains("Add"));
|
||||
Assert.IsTrue(history[1].Contains("Divide"));
|
||||
Assert.IsTrue(history[2].Contains("SquareRoot"));
|
||||
CollectionAssert.AreEqual(
|
||||
new[] { "Add(1, 2)", "Divide(10, 5)", "SquareRoot(9)" },
|
||||
history.ToArray());
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
package cache
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
type cache struct {
|
||||
mu sync.RWMutex
|
||||
values map[string]string
|
||||
}
|
||||
|
||||
func newCache() *cache {
|
||||
return &cache{values: make(map[string]string)}
|
||||
}
|
||||
|
||||
func normalize(key string) string {
|
||||
return strings.ToLower(key)
|
||||
}
|
||||
|
||||
func (c *cache) refresh(key string) {
|
||||
go func() {
|
||||
time.Sleep(10 * time.Millisecond)
|
||||
c.mu.Lock()
|
||||
defer c.mu.Unlock()
|
||||
c.values[key] = "ready"
|
||||
}()
|
||||
}
|
||||
|
||||
func (c *cache) get(key string) (string, bool) {
|
||||
c.mu.RLock()
|
||||
defer c.mu.RUnlock()
|
||||
value, ok := c.values[key]
|
||||
return value, ok
|
||||
}
|
||||
|
||||
func TestKeyNormalization(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
key string
|
||||
want string
|
||||
}{
|
||||
{name: "lowercase", key: "abc", want: "abc"},
|
||||
{name: "uppercase", key: "ABC", want: "abc"},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got := normalize(tt.key)
|
||||
if got != tt.want {
|
||||
t.Errorf("normalize(%q) = %q, want %q", tt.key, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestRefreshEventuallyStoresValue(t *testing.T) {
|
||||
cache := newCache()
|
||||
|
||||
cache.refresh("catalog")
|
||||
time.Sleep(500 * time.Millisecond)
|
||||
|
||||
if _, ok := cache.get("catalog"); !ok {
|
||||
t.Fatal("catalog was not refreshed")
|
||||
}
|
||||
}
|
||||
+1
@@ -8,5 +8,6 @@
|
||||
</PropertyGroup>
|
||||
<ItemGroup>
|
||||
<PackageReference Include="MSTest" Version="4.1.0" />
|
||||
<PackageReference Include="Microsoft.Data.Sqlite" Version="10.0.11" />
|
||||
</ItemGroup>
|
||||
</Project>
|
||||
|
||||
+119
@@ -0,0 +1,119 @@
|
||||
using Microsoft.Data.Sqlite;
|
||||
|
||||
namespace DataAccess.IntegrationTests;
|
||||
|
||||
public class User(string email, string name)
|
||||
{
|
||||
public int Id { get; set; }
|
||||
|
||||
public string Email { get; } = email;
|
||||
|
||||
public string Name { get; set; } = name;
|
||||
}
|
||||
|
||||
public sealed class PremiumUser(string email, string name) : User(email, name)
|
||||
{
|
||||
public decimal DiscountRate { get; } = 0.1m;
|
||||
}
|
||||
|
||||
public sealed class UserRepository(SqliteConnection connection)
|
||||
{
|
||||
private readonly object _gate = new();
|
||||
private readonly HashSet<int> _notifiedUserIds = [];
|
||||
|
||||
public void InitializeSchema()
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText =
|
||||
"""
|
||||
CREATE TABLE Users (
|
||||
Id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
Email TEXT NOT NULL UNIQUE,
|
||||
Name TEXT NOT NULL
|
||||
);
|
||||
""";
|
||||
command.ExecuteNonQuery();
|
||||
}
|
||||
|
||||
public void Insert(User user)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText =
|
||||
"""
|
||||
INSERT INTO Users (Email, Name) VALUES ($email, $name);
|
||||
SELECT last_insert_rowid();
|
||||
""";
|
||||
command.Parameters.AddWithValue("$email", user.Email);
|
||||
command.Parameters.AddWithValue("$name", user.Name);
|
||||
user.Id = Convert.ToInt32((long)command.ExecuteScalar()!);
|
||||
_notifiedUserIds.Add(user.Id);
|
||||
}
|
||||
}
|
||||
|
||||
public User? GetByEmail(string email)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText = "SELECT Id, Email, Name FROM Users WHERE Email = $email;";
|
||||
command.Parameters.AddWithValue("$email", email);
|
||||
using var reader = command.ExecuteReader();
|
||||
return reader.Read()
|
||||
? new User(reader.GetString(1), reader.GetString(2)) { Id = reader.GetInt32(0) }
|
||||
: null;
|
||||
}
|
||||
}
|
||||
|
||||
public void Update(User user)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText = "UPDATE Users SET Name = $name WHERE Id = $id;";
|
||||
command.Parameters.AddWithValue("$name", user.Name);
|
||||
command.Parameters.AddWithValue("$id", user.Id);
|
||||
command.ExecuteNonQuery();
|
||||
}
|
||||
}
|
||||
|
||||
public void Delete(int id)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText = "DELETE FROM Users WHERE Id = $id;";
|
||||
command.Parameters.AddWithValue("$id", id);
|
||||
command.ExecuteNonQuery();
|
||||
}
|
||||
}
|
||||
|
||||
public List<User> ListAll()
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
using var command = connection.CreateCommand();
|
||||
command.CommandText = "SELECT Id, Email, Name FROM Users ORDER BY Id;";
|
||||
using var reader = command.ExecuteReader();
|
||||
var users = new List<User>();
|
||||
while (reader.Read())
|
||||
{
|
||||
users.Add(new User(reader.GetString(1), reader.GetString(2))
|
||||
{
|
||||
Id = reader.GetInt32(0)
|
||||
});
|
||||
}
|
||||
|
||||
return users;
|
||||
}
|
||||
}
|
||||
|
||||
public bool WasNotificationSent(int userId)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
return _notifiedUserIds.Contains(userId);
|
||||
}
|
||||
}
|
||||
}
|
||||
+2
-4
@@ -1,4 +1,5 @@
|
||||
using Microsoft.VisualStudio.TestTools.UnitTesting;
|
||||
using Microsoft.Data.Sqlite;
|
||||
|
||||
namespace DataAccess.IntegrationTests;
|
||||
|
||||
@@ -77,9 +78,8 @@ public sealed class UserRepositoryIntegrationTests
|
||||
CollectionAssert.AllItemsAreNotNull(users);
|
||||
}
|
||||
|
||||
// Smell: Sleepy Test — Thread.Sleep in integration test (still a smell even for integration tests)
|
||||
[TestMethod]
|
||||
public async Task NotifyOnInsert_SendsEventAfterDelay()
|
||||
public void NotifyOnInsert_SendsEventAfterDelay()
|
||||
{
|
||||
var user = new User("dave@example.com", "Dave");
|
||||
_repository.Insert(user);
|
||||
@@ -89,7 +89,6 @@ public sealed class UserRepositoryIntegrationTests
|
||||
Assert.IsTrue(_repository.WasNotificationSent(user.Id));
|
||||
}
|
||||
|
||||
// Smell: Conditional Test Logic — if/else inside test
|
||||
[TestMethod]
|
||||
public void GetUser_ReturnsCorrectType()
|
||||
{
|
||||
@@ -108,7 +107,6 @@ public sealed class UserRepositoryIntegrationTests
|
||||
}
|
||||
}
|
||||
|
||||
// Smell: Assertion-Free Test — exercises code but asserts nothing
|
||||
[TestMethod]
|
||||
public void BulkInsert_RunsWithoutErrors()
|
||||
{
|
||||
|
||||
@@ -0,0 +1,13 @@
|
||||
describe("loadUser", () => {
|
||||
it("returns the requested user", () => {
|
||||
const loadUser = async () => ({ id: 7, name: "Ada" });
|
||||
|
||||
expect(loadUser()).resolves.toEqual({ id: 7, name: "Ada" });
|
||||
});
|
||||
|
||||
it("matches the stable snapshot", () => {
|
||||
const user = { id: 7, name: "Ada" };
|
||||
|
||||
expect(user).toMatchSnapshot();
|
||||
});
|
||||
});
|
||||
-11
@@ -21,7 +21,6 @@ public class CatalogTest {
|
||||
rng = new Random();
|
||||
}
|
||||
|
||||
// Smell: Conditional Test Logic — branching inside the test
|
||||
@Test
|
||||
void addConditionalLogic() {
|
||||
String[] skus = {"a", "b", "c"};
|
||||
@@ -35,7 +34,6 @@ public class CatalogTest {
|
||||
assertEquals(12, catalog.quantity("a") + catalog.quantity("b") + catalog.quantity("c"));
|
||||
}
|
||||
|
||||
// Smell: Mystery Guest — reads from the file system
|
||||
@Test
|
||||
void addLoadsFromFile() throws Exception {
|
||||
Path tmp = Files.createTempFile("catalog", ".txt");
|
||||
@@ -45,7 +43,6 @@ public class CatalogTest {
|
||||
assertTrue(catalog.inStock("x"));
|
||||
}
|
||||
|
||||
// Smell: Sleepy Test
|
||||
@Test
|
||||
void addSleepy() throws Exception {
|
||||
catalog.add("a", 1);
|
||||
@@ -53,13 +50,11 @@ public class CatalogTest {
|
||||
assertEquals(1, catalog.quantity("a"));
|
||||
}
|
||||
|
||||
// Smell: Assertion-Free Test
|
||||
@Test
|
||||
void addAndDoNothing() {
|
||||
catalog.add("a", 1);
|
||||
}
|
||||
|
||||
// Smell: Eager Test — calls many distinct methods
|
||||
@Test
|
||||
void doEverythingAtOnce() {
|
||||
catalog.add("a", 1);
|
||||
@@ -72,7 +67,6 @@ public class CatalogTest {
|
||||
assertEquals(6, catalog.quantity("a") + catalog.quantity("b") + catalog.quantity("c"));
|
||||
}
|
||||
|
||||
// Smell: Magic Number Test — what does 42 mean?
|
||||
@Test
|
||||
void quantityAfterAdds() {
|
||||
catalog.add("a", 41);
|
||||
@@ -80,14 +74,12 @@ public class CatalogTest {
|
||||
assertEquals(42, catalog.quantity("a"));
|
||||
}
|
||||
|
||||
// Smell: Sensitive Equality — relies on toString format
|
||||
@Test
|
||||
void toStringFormat() {
|
||||
catalog.add("a", 1);
|
||||
assertEquals("Catalog(items=1)", catalog.toString());
|
||||
}
|
||||
|
||||
// Smell: Exception Handling in Tests — manual try/catch instead of assertThrows
|
||||
@Test
|
||||
void addNullSkuManualCatch() {
|
||||
try {
|
||||
@@ -98,13 +90,11 @@ public class CatalogTest {
|
||||
}
|
||||
}
|
||||
|
||||
// Smell: General Fixture — rng never used by this test, set up by @BeforeEach
|
||||
@Test
|
||||
void quantityZeroOnUnknownSku() {
|
||||
assertEquals(0, catalog.quantity("unknown"));
|
||||
}
|
||||
|
||||
// Smell: Ignored Test
|
||||
@Test
|
||||
@Disabled("flaky, fix later")
|
||||
void disabledTest() {
|
||||
@@ -112,7 +102,6 @@ public class CatalogTest {
|
||||
assertEquals(1, catalog.quantity("x"));
|
||||
}
|
||||
|
||||
// Well-written test for contrast
|
||||
@Test
|
||||
void addThrowsOnNegativeCount() {
|
||||
IllegalArgumentException ex = assertThrows(
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
function Invoke-Worker {
|
||||
'done'
|
||||
}
|
||||
|
||||
Describe 'Worker' {
|
||||
BeforeEach {
|
||||
Mock Invoke-Worker { 'done' }
|
||||
}
|
||||
|
||||
It 'invokes the worker once' {
|
||||
Invoke-Worker
|
||||
|
||||
Should -Invoke Invoke-Worker -Times 1 -Exactly
|
||||
}
|
||||
|
||||
It 'waits for completion' {
|
||||
Invoke-Worker
|
||||
Start-Sleep -Seconds 2
|
||||
|
||||
Should -Invoke Invoke-Worker -Times 1 -Exactly
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,33 @@
|
||||
import pytest
|
||||
from unittest.mock import Mock
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("prices", "expected"),
|
||||
[
|
||||
([2, 3], 5),
|
||||
([4, 6], 10),
|
||||
],
|
||||
)
|
||||
def test_total_for_known_prices(prices, expected):
|
||||
assert sum(prices) == expected
|
||||
|
||||
|
||||
def test_callback_receives_total():
|
||||
callback = Mock()
|
||||
|
||||
callback(12)
|
||||
|
||||
callback.assert_called_once_with(12)
|
||||
|
||||
|
||||
def test_invalid_price_raises():
|
||||
with pytest.raises(ValueError, match="price"):
|
||||
raise ValueError("price must be positive")
|
||||
|
||||
|
||||
def test_all_skus_are_present():
|
||||
skus = ["A", "B", "C"]
|
||||
|
||||
for sku in skus:
|
||||
assert sku
|
||||
-10
@@ -4,9 +4,6 @@ namespace Inventory.Tests;
|
||||
|
||||
public sealed class InventoryServiceTests
|
||||
{
|
||||
// Contextually obvious numbers: we add exactly three items, then assert
|
||||
// the count is three. The literals are self-documenting and should NOT be
|
||||
// flagged as Magic Number Test.
|
||||
[Fact]
|
||||
public void AddItems_ThreeAdded_CountIsThree()
|
||||
{
|
||||
@@ -19,7 +16,6 @@ public sealed class InventoryServiceTests
|
||||
Assert.Equal(3, service.ItemCount);
|
||||
}
|
||||
|
||||
// Contextually obvious number: removing one of two items leaves one.
|
||||
[Fact]
|
||||
public void RemoveItem_FromTwo_LeavesOne()
|
||||
{
|
||||
@@ -32,8 +28,6 @@ public sealed class InventoryServiceTests
|
||||
Assert.Equal(1, service.ItemCount);
|
||||
}
|
||||
|
||||
// Reasoned skip: the annotation documents WHY it is skipped and links a
|
||||
// tracking issue. This is less concerning than an unexplained skip.
|
||||
[Fact(Skip = "Tracked by #1487 - blocked on the warehouse API redesign")]
|
||||
public void Reserve_AcrossWarehouses_BalancesStock()
|
||||
{
|
||||
@@ -45,8 +39,6 @@ public sealed class InventoryServiceTests
|
||||
Assert.True(reserved);
|
||||
}
|
||||
|
||||
// Bare skip: no reason given at all. The reader has no idea why it is
|
||||
// disabled or whether the underlying issue is tracked anywhere.
|
||||
[Fact(Skip = "skip")]
|
||||
public void Restock_FromSupplier_UpdatesQuantities()
|
||||
{
|
||||
@@ -57,7 +49,6 @@ public sealed class InventoryServiceTests
|
||||
Assert.Equal(50, service.QuantityOf("SKU-9"));
|
||||
}
|
||||
|
||||
// Sleepy Test: real smell with a clear high-confidence severity rationale.
|
||||
[Fact]
|
||||
public void Replenish_AsyncJob_Completes()
|
||||
{
|
||||
@@ -70,7 +61,6 @@ public sealed class InventoryServiceTests
|
||||
Assert.True(service.WasReplenished("SKU-1"));
|
||||
}
|
||||
|
||||
// Assertion-Free Test: real smell, exercises code but verifies nothing.
|
||||
[Fact]
|
||||
public void Audit_RunsWithoutError()
|
||||
{
|
||||
|
||||
+2
-14
@@ -19,7 +19,6 @@ public sealed class OrderProcessorTests
|
||||
_logger = new FakeLogger();
|
||||
}
|
||||
|
||||
// Smell: Conditional Test Logic — uses if/else inside test
|
||||
[TestMethod]
|
||||
public void ProcessOrder_SetsCorrectStatus()
|
||||
{
|
||||
@@ -38,7 +37,6 @@ public sealed class OrderProcessorTests
|
||||
}
|
||||
}
|
||||
|
||||
// Smell: Assertion-Free Test — no assertions at all
|
||||
[TestMethod]
|
||||
public void ProcessOrder_CompletesWithoutError()
|
||||
{
|
||||
@@ -47,7 +45,6 @@ public sealed class OrderProcessorTests
|
||||
processor.ProcessOrder(order);
|
||||
}
|
||||
|
||||
// Smell: Eager Test — calls many distinct production methods
|
||||
[TestMethod]
|
||||
public void OrderProcessor_FullWorkflow_Succeeds()
|
||||
{
|
||||
@@ -65,7 +62,6 @@ public sealed class OrderProcessorTests
|
||||
Assert.AreEqual("Completed", order.Status);
|
||||
}
|
||||
|
||||
// Smell: Magic Number Test — unexplained numeric literals
|
||||
[TestMethod]
|
||||
public void CalculateTotal_ReturnsCorrectAmount()
|
||||
{
|
||||
@@ -86,7 +82,6 @@ public sealed class OrderProcessorTests
|
||||
Assert.AreEqual(269.78m, order.GrandTotal);
|
||||
}
|
||||
|
||||
// Smell: Sleepy Test — uses Thread.Sleep
|
||||
[TestMethod]
|
||||
public void ProcessOrder_AsyncNotification_IsSent()
|
||||
{
|
||||
@@ -100,7 +95,6 @@ public sealed class OrderProcessorTests
|
||||
Assert.IsTrue(_email.WasNotificationSent(order.Id));
|
||||
}
|
||||
|
||||
// Smell: Exception Handling in Test — try/catch instead of Assert.ThrowsException
|
||||
[TestMethod]
|
||||
public void ProcessOrder_EmptyOrder_ThrowsValidationError()
|
||||
{
|
||||
@@ -118,9 +112,8 @@ public sealed class OrderProcessorTests
|
||||
}
|
||||
}
|
||||
|
||||
// Smell: Sensitive Equality — uses ToString() for comparison
|
||||
[TestMethod]
|
||||
public void GetOrderSummary_ReturnsFormattedString()
|
||||
public void GetOrderSummary_ReturnsOrderDetails()
|
||||
{
|
||||
var processor = new OrderProcessor(_db, _email, _inventory);
|
||||
var order = new Order
|
||||
@@ -135,7 +128,6 @@ public sealed class OrderProcessorTests
|
||||
Assert.AreEqual("Order ORD-001: 1 item(s), Total: $99.99", summary.ToString());
|
||||
}
|
||||
|
||||
// Smell: Mystery Guest — reads from file system
|
||||
[TestMethod]
|
||||
public void ImportOrders_FromCsv_ParsesCorrectly()
|
||||
{
|
||||
@@ -147,16 +139,12 @@ public sealed class OrderProcessorTests
|
||||
Assert.AreEqual(5, orders.Count);
|
||||
}
|
||||
|
||||
// Smell: General Fixture — _logger is initialized in Setup but never used by any test
|
||||
// (All tests above use _db, _email, _inventory but none use _logger)
|
||||
|
||||
// Clean test for contrast — this one has no smells
|
||||
[TestMethod]
|
||||
public void ValidateOrder_NullOrder_ThrowsArgumentNullException()
|
||||
{
|
||||
var processor = new OrderProcessor(_db, _email, _inventory);
|
||||
|
||||
var ex = Assert.ThrowsException<ArgumentNullException>(
|
||||
var ex = Assert.ThrowsExactly<ArgumentNullException>(
|
||||
() => processor.ValidateOrder(null!));
|
||||
|
||||
Assert.AreEqual("order", ex.ParamName);
|
||||
|
||||
+103
@@ -0,0 +1,103 @@
|
||||
namespace OrderService.Tests;
|
||||
|
||||
public sealed class FakeDatabase
|
||||
{
|
||||
}
|
||||
|
||||
public sealed class FakeInventory
|
||||
{
|
||||
}
|
||||
|
||||
public sealed class FakeLogger
|
||||
{
|
||||
}
|
||||
|
||||
public sealed class FakeEmailSender
|
||||
{
|
||||
private readonly HashSet<string> _notifiedOrderIds = [];
|
||||
|
||||
public void RecordNotification(string orderId) => _notifiedOrderIds.Add(orderId);
|
||||
|
||||
public bool WasNotificationSent(string orderId) => _notifiedOrderIds.Contains(orderId);
|
||||
}
|
||||
|
||||
public sealed class Order
|
||||
{
|
||||
public string Id { get; set; } = "ORD-001";
|
||||
|
||||
public List<OrderItem> Items { get; } = [];
|
||||
|
||||
public decimal TotalAmount { get; set; }
|
||||
|
||||
public decimal TaxAmount { get; set; }
|
||||
|
||||
public decimal GrandTotal { get; set; }
|
||||
|
||||
public string Status { get; set; } = "Pending";
|
||||
}
|
||||
|
||||
public sealed record OrderItem(string Sku, int Quantity);
|
||||
|
||||
public sealed record CreditCard(string Number);
|
||||
|
||||
public sealed record OrderResult(decimal TotalAmount, string Status);
|
||||
|
||||
public sealed record OrderSummary(string Id, int ItemCount, decimal TotalAmount)
|
||||
{
|
||||
public override string ToString() =>
|
||||
FormattableString.Invariant(
|
||||
$"Order {Id}: {ItemCount} item(s), Total: ${TotalAmount:0.00}");
|
||||
}
|
||||
|
||||
public sealed class ValidationException(string message) : Exception(message)
|
||||
{
|
||||
}
|
||||
|
||||
public sealed class OrderProcessor(
|
||||
FakeDatabase database,
|
||||
FakeEmailSender email,
|
||||
FakeInventory inventory)
|
||||
{
|
||||
public OrderResult ProcessOrder(Order order)
|
||||
{
|
||||
ValidateOrder(order);
|
||||
return new OrderResult(order.TotalAmount, "StandardProcessed");
|
||||
}
|
||||
|
||||
public void ProcessOrderAsync(Order order)
|
||||
{
|
||||
ProcessOrder(order);
|
||||
email.RecordNotification(order.Id);
|
||||
}
|
||||
|
||||
public void ValidateOrder(Order? order)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(order);
|
||||
if (order.Items.Count == 0)
|
||||
{
|
||||
throw new ValidationException("Order must contain at least one item");
|
||||
}
|
||||
}
|
||||
|
||||
public void CalculateTotal(Order order)
|
||||
{
|
||||
order.TotalAmount = 247.50m;
|
||||
order.TaxAmount = 22.28m;
|
||||
order.GrandTotal = 269.78m;
|
||||
}
|
||||
|
||||
public void ApplyDiscount(Order order, string code) => _ = (order, code);
|
||||
|
||||
public void ReserveInventory(Order order) => _ = (order, inventory);
|
||||
|
||||
public void ProcessPayment(Order order, CreditCard card) => _ = (order, card, database);
|
||||
|
||||
public void SendConfirmation(Order order) => _ = order;
|
||||
|
||||
public void UpdateOrderHistory(Order order) => order.Status = "Completed";
|
||||
|
||||
public OrderSummary GetOrderSummary(Order order) =>
|
||||
new(order.Id, order.Items.Count, order.TotalAmount);
|
||||
|
||||
public List<Order> ImportOrders(string csv) => [new(), new(), new(), new(), new()];
|
||||
}
|
||||
Reference in New Issue
Block a user