docs: consolidate hooks, trim duplication, and restructure sidebar#5024
docs: consolidate hooks, trim duplication, and restructure sidebar#5024
Conversation
Merge hooks-setup.md and hooks-cleanup.md into a single hooks.md, add a data-driven testing overview page, trim verbose tabbed examples across 5 files, consolidate duplicated "awaiting assertions" content into links to the canonical page, and restructure the sidebar (merge Comparing + Migration, relocate Guides pages, add Test Data overview entry). Net reduction of ~750 lines while preserving all content.
There was a problem hiding this comment.
Documentation Review — docs: consolidate hooks, trim duplication, and restructure sidebar
Overall: This is a well-intentioned documentation cleanup. The net -441 lines is meaningful for long-term maintainability, the hook consolidation makes structural sense, and the new data-driven-overview.md decision table is a genuinely useful addition. I have a few concerns worth addressing before merging.
✅ What Works Well
- Hook page consolidation (
hooks-setup.md+hooks-cleanup.md→hooks.md): These two pages were inherently coupled. A single page with clearly delineated Setup/Cleanup sections is better than two pages that each have to explain context from the other. data-driven-overview.md: The decision table is excellent. "Which data attribute should I use?" is a real first-time user question that wasn't answered anywhere prominent. Placing it first in the Test Data sidebar category is exactly right.- Table-based comparison for
IAsyncInitializervsIAsyncDiscoveryInitializer: Tables are far more scannable than nested tabs for binary comparisons. Good call. - Link updates: Comprehensive —
lifecycle.md,things-to-know.md,artifacts.md, andintro.mdare all updated consistently.
⚠️ Concerns
1. "Forgetting to Await" loses its most important example in getting-started.md
The original getting-started.md had a visible code snippet right on the page:
// ✅ Correct - awaited
await Assert.That(result).IsEqualTo(42);
// ❌ Wrong - will cause compiler warning
Assert.That(result).IsEqualTo(42);This is replaced with a single-sentence pointer to awaiting.md. This is the first page new users land on — the one place where an inline example of the most common beginner mistake has the highest value. The pattern of "show it here, link for more detail" is better than "just link." Consider keeping the two-line snippet while still linking to awaiting.md for the rationale.
Same concern applies to writing-your-first-test.md: removing the "Important Notes" about async Task vs void/async void in favour of a :::tip link means a new user writing their first test may skip the warning entirely.
Why this matters architecturally: First-contact pages (getting-started, writing-your-first-test) serve a different audience than reference pages. Trimming them to brevity optimises for readers who already understand the concepts, at the expense of readers who need them most.
2. Unchecked test plan items
The PR description includes verification steps that are not checked off:
- Spot-check sidebar renders correctly in dev mode
- Verify new
hooks.mdanddata-driven-overview.mdpages render properly - Confirm no content was lost — only deduplicated or condensed
These should be completed (or explicitly noted as "out of scope / handled by CI") before merge. Broken sidebar rendering or a missing anchor in hooks.md could silently break the docs site.
3. intro.md: merging 3 navigation bullets into 1 reduces discoverability
- - Comparing Frameworks
- - Migration
- - Guides
+ - Comparing & MigratingMigration guides are a high-value entry point for users switching from xUnit/NUnit/MSTest — arguably the audience most likely to evaluate TUnit. Collapsing "Migration" into a combined bullet reduces its prominence at the one place (the intro page) where it matters most. Consider keeping migration as a distinct bullet, or at least ensuring the sidebar position for migration guides is clearly discoverable.
Minor Notes
- The
troubleshooting.mdreduction ("Why do I have to await all assertions?" from 3 paragraphs to 1 sentence) is fine since troubleshooting readers are already invested enough to click through. things-to-know.md— the inline-commented single snippet replacing the bad/good tabs is a good tradeoff; the inline comment style conveys the same information more compactly.
Summary: The structural changes (hook consolidation, sidebar restructure, new overview page) are solid improvements. The main ask is to reconsider removing inline code examples from the two first-contact pages (getting-started.md, writing-your-first-test.md) where they serve new users who are least likely to follow a link, and to close out the unchecked test plan items.
… bullet Address review feedback: - Restore inline await/no-await code snippet in assertions/getting-started.md - Restore Important Notes bullets in writing-your-first-test.md - Split "Comparing & Migrating" back into separate bullets in intro.md so migration guides remain prominent for evaluating users
There was a problem hiding this comment.
Follow-up Review
Previous Concerns — Status
All three concerns from the prior review have been addressed in the follow-up commit:
- ✅
getting-started.md: Inline await/no-await code snippet retained - ✅
writing-your-first-test.md: Important Notes bullets restored - ✅
intro.md: Migration is kept as its own bullet, preserving discoverability
Good turnaround on those.
New Issue: Incorrect Attribute Name in data-driven-overview.md
The new decision table and Matrix example use [Matrix] as the method-level attribute, but the correct attribute is [MatrixDataSource]:
Decision table (incorrect):
| All parameter combinations | `[Matrix]` | Matrix Tests |
Code example (incorrect):
[Test]
[Matrix] // ❌ should be [MatrixDataSource]
public async Task Multiply(
[Matrix(2, 3)] int a,
[Matrix(4, 5)] int b)The correct pattern, as used throughout the codebase (e.g., CloudShop.Tests, TUnit.Templates.Tests, TUnit.Analyzers.Tests):
[Test]
[MatrixDataSource] // ✅ method-level attribute
public async Task Multiply(
[Matrix(2, 3)] int a, // ✅ parameter-level attributes
[Matrix(4, 5)] int b)
{
await Assert.That(a * b).IsGreaterThan(0);
}The decision table entry should also read [MatrixDataSource] (the method attribute that a user would reach for) rather than [Matrix] (the parameter attribute). This needs fixing before merge — the overview page is a high-traffic entry point and a wrong attribute name here will frustrate new users.
Minor: things-to-know.md — only the bad case shown
The replaced tabbed section showed both bad (instance field) and good (static field) code. The new version shows only the bad case with an inline comment, and the fix is text-only. This is a lower priority than the Matrix issue, but if the goal is to help someone understand what to write instead, a one-liner private static int _value; // ✅ shared across instances would complete the picture.
Unchecked Test Plan Items
- [ ] Spot-check sidebar renders correctly in dev mode
- [ ] Verify new hooks.md and data-driven-overview.md pages render properly
- [ ] Confirm no content was lost — only deduplicated or condensed
These are still open. If they have been done locally and CI is the only remaining gate, just check them off to signal intent to reviewers.
Overall: The structural improvements are solid and the prior feedback was addressed promptly. The one blocker is the [Matrix] → [MatrixDataSource] fix in the new overview page.
- Decision table and code example incorrectly used [Matrix] as the method-level attribute; the correct attribute is [MatrixDataSource] - Add explicit `private static int _value;` to the fix note in things-to-know.md for clarity
There was a problem hiding this comment.
Follow-up Review (Round 3)
Previous Concerns — Status
From the second review:
- ✅
[Matrix]→[MatrixDataSource]— Fixed. The decision table entry now reads[MatrixDataSource]and the code example uses[MatrixDataSource]as the method-level attribute with[Matrix(...)]on parameters. The blocker is resolved. ⚠️ Unchecked test plan items — Still unchecked (see below).things-to-know.mdgood example — Still text-only. Low priority, but noted again below.
Unchecked Test Plan Items (still open)
- [ ] Spot-check sidebar renders correctly in dev mode
- [ ] Verify new hooks.md and data-driven-overview.md pages render properly
- [ ] Confirm no content was lost — only deduplicated or condensed
If these have been done locally, just check them off. If CI's npm run build is the only gate being relied on, that's worth calling out explicitly — a build can pass while a sidebar entry points to a missing anchor or a renamed page renders with broken relative links.
Minor: things-to-know.md — good case still text-only
The current version shows only the problematic pattern:
private int _value; // ❌ reset to 0 for every test — different instances!The fix is prose: "Use private static int _value;". Adding a one-line companion would complete the picture without re-introducing the tab complexity:
private int _value; // ❌ reset for every test — different instances
private static int _value; // ✅ shared across instances (use with care)Not a blocker, but the previous tabs explicitly showed both — dropping to prose-only is a slight regression in clarity for this particular concept.
Overall
The structural work is solid: hook consolidation, data-driven-overview.md, table-based comparisons, and link fixes are all improvements over the original. The one blocker ([MatrixDataSource]) has been addressed. The remaining items are minor process hygiene (test plan checkboxes) and a low-priority clarity note. This is close to merge-ready — just close out those checklist items.
Summary
hooks-setup.md+hooks-cleanup.mdinto a singlehooks.md(~210 lines, down from 451 combined), replacing duplicated method signatures, parameter tables, and 3 verbose tabbed BAD/GOOD sections with concise do/don't bulletsdata-driven-overview.mdwith a decision table and quick examples for all 8 data approaches, added as the first item in the Test Data sidebar categoryproperty-injection.md(~137 lines → ~45), tabbed BAD/GOOD inassertions/getting-started.md(~126 lines → ~4), and tabbed BAD/GOOD inthings-to-know.md(~50 lines → ~15)assertions/awaiting.mdlifecycle.md,things-to-know.md,artifacts.md, andintro.mdNet change: -441 lines (348 added, 789 removed) across 14 files (2 new, 2 deleted, 10 edited).
Test plan
npm run buildindocs/passes with zero errors and no broken link warningshooks.mdanddata-driven-overview.mdpages render properly