From 61375805ca3a22fdca88120ddebf68133882e535 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 05:04:05 -0400 Subject: [PATCH 01/14] Increment 1: test default behavior --- docs/tasks/cache-unchecked-external-links.md | 59 ++++++++++++++++++-- htmltest/check-link-cache_test.go | 24 ++++++++ htmltest/test_helpers_extra_test.go | 23 ++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 htmltest/test_helpers_extra_test.go diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index 0147265..b0aa802 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -160,10 +160,59 @@ Two modifications needed: - **No need for backward compat with dev/main features**: We can change `RetryCachedErrors` behavior if needed +## Incremental Implementation Strategy + +### Increment 1: Test #1 - Default Behavior (Baseline) +**Purpose**: Establish regression test + +- Write test: Verify `CacheAllExternal: false` doesn't cache discovered links +- Run test: Should PASS immediately (tests current behavior) +- No code needed: This is baseline +- Benefit: Protects against future regressions + +### Increment 2: Infrastructure + Test #2 - Discovery Mode +**Purpose**: Add config option + core discovery feature + +- Add config: `CacheAllExternal bool` to Options struct +- Write test: Discovery mode caches with `StatusUnchecked` +- Run test: RED (config exists but no caching code) +- Implement: Add discovery caching in `checkExternal()` +- Run test: GREEN +- **This is the core feature** + +### Increment 3: Test #4 - Ignored URLs +**Purpose**: Verify edge case works + +- Write test: Ignored URLs not cached +- Run test: Likely PASS (existing `isURLIgnored()` should work) +- If RED: Fix discovery code to respect ignore patterns +- Benefit: Validates design assumption + +### Increment 4: Test #5 - Query String Stripping +**Purpose**: Verify edge case works + +- Write test: Query strings stripped before caching +- Run test: Likely PASS (existing logic should work) +- If RED: Adjust operation order +- Benefit: Validates design assumption + +### Increment 5: Test #3 - Timeout Caching +**Purpose**: Complete second dimension of feature + +- Write test: Timeouts cached when `CacheAllExternal: true` +- Run test: RED (still checks `RetryCachedErrors`) +- Implement: Change timeout caching condition +- Run test: GREEN +- **Completes the feature** + +**Rationale for order**: Infrastructure first (#2), validate assumptions while fresh +(#4, #5), then complete with timeout caching (#3). + ## To-dos -- [ ] Write test cases for CacheAllExternal feature (TDD) -- [ ] Add CacheAllExternal field to Options struct and DefaultOptions() -- [ ] Implement discovery mode caching (CheckExternal: false) -- [ ] Implement timeout caching (CheckExternal: true) -- [ ] Add CacheAllExternal to README configuration table +- [ ] Increment 1: Baseline test (default behavior) +- [ ] Increment 2: Config option + discovery mode +- [ ] Increment 3: Ignored URLs edge case +- [ ] Increment 4: Query stripping edge case +- [ ] Increment 5: Timeout caching +- [ ] Update README configuration table diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index adfa6bc..865f9cf 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -10,6 +10,10 @@ import ( // Feature: RetryCachedErrors // Added by @chalin +// ======================================== +// External Error Caching Tests +// ======================================== + // TestExternalErrorCached : Test that URLs with HTTP error status codes (such // as 404) are saved to the refcache. func TestExternalErrorCached(t *testing.T) { @@ -159,3 +163,23 @@ func TestTimeoutCachedMessage(t *testing.T) { tExpectIssueCount(t, hT2, 1) tExpectIssue(t, hT2, "request exceeded our ExternalTimeout (cached)", 1) } + +// ======================================== +// CacheAllExternal Tests +// ======================================== + +// TestCacheAllExternalDisabled : Test that external links are NOT cached when +// CacheAllExternal is false (default behavior). +// This is a regression test to ensure the default behavior doesn't change. +func TestCacheAllExternalDisabled(t *testing.T) { + // Run with CheckExternal disabled and CacheAllExternal disabled (defaults) + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/brokenLinkExternalSingle.html", + map[string]interface{}{"CheckExternal": false, "EnableCache": true}) + tExpectIssueCount(t, hT, 0) // No errors since external checking is disabled + + // Verify the external link was NOT cached (default behavior) + _, ok := hT.refCache.Get("http://www.asdo3IRJ395295jsingrkrg4.com") + if ok { + t.Error("external links should NOT be cached when CacheAllExternal is false (default)") + } +} diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go new file mode 100644 index 0000000..0c7dfbf --- /dev/null +++ b/htmltest/test_helpers_extra_test.go @@ -0,0 +1,23 @@ +package htmltest + +import ( + "os" + + "github.com/imdario/mergo" +) + +// Test fixture helpers + +// tTestFileOptsFromCleanOutputDir runs a test after having removed the default +// output directory. This is ensures a clean fixture for tests that might be +// influenced by the output directory content, such as the refcache file. +func tTestFileOptsFromCleanOutputDir(filename string, tOpts map[string]interface{}) *HTMLTest { + tRemoveOutputDir(tOpts) + return tTestFileOpts(filename, tOpts) +} + +func tRemoveOutputDir(tOpts map[string]interface{}) { + opts := DefaultOptions() + mergo.MergeWithOverwrite(&opts, tOpts) + os.RemoveAll(opts["OutputDir"].(string)) +} From 23fd246d743d13b51793b14a97a837f46435dab8 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 05:09:14 -0400 Subject: [PATCH 02/14] Update check-link-cache_test.go: ensure all tests start from a clean fixture --- docs/tasks/cache-unchecked-external-links.md | 15 ++++++++++++--- htmltest/check-link-cache_test.go | 14 +++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index b0aa802..3601dfe 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -1,7 +1,7 @@ --- title: CacheAllExternal Feature date: 2025-10-18 -lastmod: 2025-10-18 +lastmod: 2025-10-19 status: in-progress --- @@ -99,6 +99,10 @@ For each behavior in the matrix below: **File**: `htmltest/check-link-cache_test.go` +**Best Practice**: Use `tTestFileOptsFromCleanOutputDir()` for the first test +call in each test function to ensure test isolation with a clean cache +directory. Use `tTestFileOpts()` for subsequent calls within the same test. + ### 2. Add Configuration Option **File**: `htmltest/options.go` @@ -163,6 +167,7 @@ Two modifications needed: ## Incremental Implementation Strategy ### Increment 1: Test #1 - Default Behavior (Baseline) + **Purpose**: Establish regression test - Write test: Verify `CacheAllExternal: false` doesn't cache discovered links @@ -171,6 +176,7 @@ Two modifications needed: - Benefit: Protects against future regressions ### Increment 2: Infrastructure + Test #2 - Discovery Mode + **Purpose**: Add config option + core discovery feature - Add config: `CacheAllExternal bool` to Options struct @@ -181,6 +187,7 @@ Two modifications needed: - **This is the core feature** ### Increment 3: Test #4 - Ignored URLs + **Purpose**: Verify edge case works - Write test: Ignored URLs not cached @@ -189,6 +196,7 @@ Two modifications needed: - Benefit: Validates design assumption ### Increment 4: Test #5 - Query String Stripping + **Purpose**: Verify edge case works - Write test: Query strings stripped before caching @@ -197,6 +205,7 @@ Two modifications needed: - Benefit: Validates design assumption ### Increment 5: Test #3 - Timeout Caching + **Purpose**: Complete second dimension of feature - Write test: Timeouts cached when `CacheAllExternal: true` @@ -205,8 +214,8 @@ Two modifications needed: - Run test: GREEN - **Completes the feature** -**Rationale for order**: Infrastructure first (#2), validate assumptions while fresh -(#4, #5), then complete with timeout caching (#3). +**Rationale for order**: Infrastructure first (#2), validate assumptions while +fresh (#4, #5), then complete with timeout caching (#3). ## To-dos diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 865f9cf..ddab2d8 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -17,7 +17,7 @@ import ( // TestExternalErrorCached : Test that URLs with HTTP error status codes (such // as 404) are saved to the refcache. func TestExternalErrorCached(t *testing.T) { - hT := tTestFileOpts("fixtures/images/imageExternal404.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/images/imageExternal404.html", map[string]interface{}{"VCREnable": true, "EnableCache": true}) tExpectIssueCount(t, hT, 1) @@ -36,7 +36,7 @@ func TestExternalErrorCached(t *testing.T) { // (404, etc.) are retried on subsequent runs. func TestExternalErrorCachedRetried(t *testing.T) { // First run: populate cache with 404 - hT := tTestFileOpts("fixtures/images/imageExternal404.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/images/imageExternal404.html", map[string]interface{}{"VCREnable": true, "EnableCache": true}) tExpectIssueCount(t, hT, 1) @@ -59,7 +59,7 @@ func TestExternalErrorCachedRetried(t *testing.T) { // Uses elements for variety. func TestExternalBrokenRetryCachedErrorsDisabled(t *testing.T) { // First run: populate cache with 404 - hT := tTestFileOpts("fixtures/generic/citeBroken.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/generic/citeBroken.html", map[string]interface{}{"VCREnable": true, "EnableCache": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 4) // 4 broken citations in the fixture @@ -85,7 +85,7 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { tSkipShortExternal(t) // First run: timeout occurs - hT := tTestFileOpts("fixtures/links/ip_timeout.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "LogLevel": issues.LevelDebug}) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) @@ -108,7 +108,7 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { // when RetryCachedErrors is false. func TestTimeoutIsCached(t *testing.T) { tSkipShortExternal(t) - hT := tTestFileOpts("fixtures/links/ip_timeout.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) @@ -129,7 +129,7 @@ func TestTimeoutCachedReused(t *testing.T) { tSkipShortExternal(t) // First run: cause and cache a timeout - hT := tTestFileOpts("fixtures/links/ip_timeout.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 1) @@ -153,7 +153,7 @@ func TestTimeoutCachedMessage(t *testing.T) { tSkipShortExternal(t) // First run: cause and cache a timeout - hT := tTestFileOpts("fixtures/links/ip_timeout.html", + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 1) From e4cd148a5d903231f23afbb038dcbfcaaa6acb74 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 05:43:17 -0400 Subject: [PATCH 03/14] Add TestTimeoutNotCachedWithRetryCacheErrorsOnly, it is green --- docs/tasks/cache-unchecked-external-links.md | 110 +++++++++++++++---- htmltest/check-link-cache_test.go | 30 ++++- htmltest/check-link.go | 7 +- 3 files changed, 117 insertions(+), 30 deletions(-) diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index 3601dfe..abcd242 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -44,14 +44,30 @@ Example workflow: 3. **`CacheAllExternal: true | false`** (NEW) - Whether to cache everything including timeouts and unchecked links +### RetryCachedErrors behavior cleanup + +`RetryCachedErrors` currently conflates two concerns: + +1. Whether to retry cached errors (its intended purpose) +2. Whether to cache timeouts (side effect when set to `false`) + +With `CacheAllExternal`, we can separate these orthogonal concerns: + +- `CacheAllExternal` controls **what gets cached** (just checked results, or + everything including timeouts/unchecked) +- `RetryCachedErrors` controls **retry behavior only** (use cached errors, or + retry them) + +This cleanup must happen BEFORE implementing the discovery mode feature. + ### Complete Behavior Matrix | CheckExternal | CacheAllExternal | RetryCachedErrors | Links Checked? | Errors Retried? | What Gets Cached | Use Case | | ------------- | ---------------- | ----------------- | -------------- | --------------- | ----------------- | ------------------------------------ | | `true` | `false` | `true` | ✓ | ✓ | 200, 4XX | **Default/Legacy** | -| `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Reuse errors, retry timeouts | +| `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Failed links are not retried | | `true` | `true` | `true` | ✓ | ✓ | 200, 4XX, TSC[^1] | Cache timeouts, but retry | -| `true` | `true` | `false` | ✓ | ✗ | 200, 404, TSC[^1] | **Fast re-runs** - cache & reuse all | +| `true` | `true` | `false` | ✓ | ✗ | 200, 4XX, TSC[^1] | **Fast re-runs** - cache & reuse all | | `false` | `false` | (N/A) | ✗ | N/A | Nothing | **Default skip** - no cache | | `false` | `true` | (N/A) | ✗ | N/A | unchecked links | **Link discovery** | @@ -64,6 +80,8 @@ Example workflow: - `CacheAllExternal` extends caching behavior in **both** modes: - When `CheckExternal: true` → Also caches timeouts (as `StatusTimeout`) - When `CheckExternal: false` → Caches discovered links (as `StatusUnchecked`) +- `RetryCachedErrors` now has clean, single-purpose semantics (retry vs. reuse + cached errors) - When `CheckExternal: false`, `RetryCachedErrors` has no effect (nothing to retry) - See `@docs/tasks/design.md` for status code details @@ -166,27 +184,64 @@ Two modifications needed: ## Incremental Implementation Strategy -### Increment 1: Test #1 - Default Behavior (Baseline) +### Phase 0: Cleanup `RetryCachedErrors` Semantics + +**Purpose**: Separate concerns before adding new functionality + +#### Step 0a: Add config option + +- Add `CacheAllExternal bool` to `Options` struct +- Add default value `false` in `DefaultOptions()` +- Run existing tests: Should PASS (option exists but unused) + +#### Step 0b: Update existing timeout tests + +- Update 3 existing tests to use `CacheAllExternal: true` instead of + `RetryCachedErrors: false`: + - `TestTimeoutIsCached` + - `TestTimeoutCachedReused` + - `TestTimeoutCachedMessage` +- Run tests: RED (tests expect new behavior, code uses old) + +#### Step 0c: Refactor timeout caching code + +- In `check-link.go`, change timeout caching condition from: + - OLD: `if !hT.opts.RetryCachedErrors` + - NEW: `if hT.opts.CacheAllExternal` +- Run tests: GREEN (existing tests verify behavior) -**Purpose**: Establish regression test +#### Step 0d: Verify row 2 behavior + +- Ensure `TestTimeoutNotCachedByDefault` still passes +- This test verifies row 2: `CacheAllExternal: false` + + `RetryCachedErrors: false` = NO timeout caching + +**Benefit**: Clean semantics established, existing tests ensure no regression + +--- + +### Phase 1: Discovery Mode Feature + +#### Increment 1: Test #1 - Default Behavior (Baseline) + +**Purpose**: Establish regression test for row 5 - Write test: Verify `CacheAllExternal: false` doesn't cache discovered links - Run test: Should PASS immediately (tests current behavior) - No code needed: This is baseline - Benefit: Protects against future regressions -### Increment 2: Infrastructure + Test #2 - Discovery Mode +#### Increment 2: Test #2 - Discovery Mode (Core Feature) -**Purpose**: Add config option + core discovery feature +**Purpose**: Implement row 6 - the main use case -- Add config: `CacheAllExternal bool` to Options struct - Write test: Discovery mode caches with `StatusUnchecked` -- Run test: RED (config exists but no caching code) +- Run test: RED (config exists but no discovery caching code) - Implement: Add discovery caching in `checkExternal()` - Run test: GREEN - **This is the core feature** -### Increment 3: Test #4 - Ignored URLs +#### Increment 3: Test #4 - Ignored URLs **Purpose**: Verify edge case works @@ -195,7 +250,7 @@ Two modifications needed: - If RED: Fix discovery code to respect ignore patterns - Benefit: Validates design assumption -### Increment 4: Test #5 - Query String Stripping +#### Increment 4: Test #5 - Query String Stripping **Purpose**: Verify edge case works @@ -204,24 +259,35 @@ Two modifications needed: - If RED: Adjust operation order - Benefit: Validates design assumption -### Increment 5: Test #3 - Timeout Caching +#### Increment 5: Verify Row 3 & Row 4 Coverage -**Purpose**: Complete second dimension of feature +**Purpose**: Ensure timeout+retry combinations work -- Write test: Timeouts cached when `CacheAllExternal: true` -- Run test: RED (still checks `RetryCachedErrors`) -- Implement: Change timeout caching condition -- Run test: GREEN -- **Completes the feature** +- Verify `TestTimeoutCachedReused` covers row 4 (cache + reuse) +- Consider if we need explicit test for row 3 (cache + retry) +- Existing test infrastructure may already cover this -**Rationale for order**: Infrastructure first (#2), validate assumptions while -fresh (#4, #5), then complete with timeout caching (#3). +**Rationale for order**: Clean up semantics first (Phase 0), then add +infrastructure and core discovery feature (Phase 1), validate assumptions while +fresh, verify complete matrix coverage. ## To-dos -- [ ] Increment 1: Baseline test (default behavior) -- [ ] Increment 2: Config option + discovery mode +### Phase 0: Cleanup + +- [ ] Step 0a: Add `CacheAllExternal` config option +- [ ] Step 0b: Update 3 existing timeout tests +- [ ] Step 0c: Refactor timeout caching code +- [ ] Step 0d: Verify row 2 behavior + +### Phase 1: Discovery Mode + +- [ ] Increment 1: Baseline test (default behavior - row 5) +- [ ] Increment 2: Discovery mode test + implementation (row 6) - [ ] Increment 3: Ignored URLs edge case - [ ] Increment 4: Query stripping edge case -- [ ] Increment 5: Timeout caching +- [ ] Increment 5: Verify row 3 & 4 coverage + +### Documentation + - [ ] Update README configuration table diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index ddab2d8..6e91c42 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -85,14 +85,13 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { tSkipShortExternal(t) // First run: timeout occurs - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "LogLevel": issues.LevelDebug}) + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "LogLevel": issues.LevelDebug} + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) // Second run: should retry (not use cache) because RetryCachedErrors is true (default) - hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "LogLevel": issues.LevelDebug}) + hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT2, 1) // Verify it retried (should see "fresh" and "hitting", not "from cache") @@ -104,6 +103,26 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { } } +// TestTimeoutNotCachedWithRetryCacheErrorsOnly : Test that timeouts are NOT cached +// when ONLY RetryCachedErrors is false (without CacheAllExternal). +// This verifies the cleanup: RetryCachedErrors no longer controls timeout caching. +func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { + tSkipShortExternal(t) + + // Run with RetryCachedErrors: false but CacheAllExternal: false (default) + // After cleanup, this should NOT cache timeouts + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", + map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) + tExpectIssueCount(t, hT, 1) + tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) + + // Verify the timeout was NOT cached (new behavior after cleanup) + _, ok := hT.refCache.Get("http://5.6.7.8") + if ok { + t.Error("timeout should NOT be cached when only RetryCachedErrors is false (without CacheAllExternal)") + } +} + // TestTimeoutIsCached : Test that URLs that timeout are saved to the refcache // when RetryCachedErrors is false. func TestTimeoutIsCached(t *testing.T) { @@ -124,7 +143,8 @@ func TestTimeoutIsCached(t *testing.T) { } // TestTimeoutCachedReused : Test that cached timeout results are reused on -// subsequent runs without retrying the URL when RetryCachedErrors is false. +// subsequent runs without retrying the URL when RetryCachedErrors is false and +// CacheAllExternal is true. TODO: fix me. func TestTimeoutCachedReused(t *testing.T) { tSkipShortExternal(t) diff --git a/htmltest/check-link.go b/htmltest/check-link.go index befa2e7..d26a4f5 100644 --- a/htmltest/check-link.go +++ b/htmltest/check-link.go @@ -199,9 +199,10 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { if err != nil { if strings.Contains(err.Error(), "Client.Timeout") { - if !hT.opts.RetryCachedErrors { - hT.refCache.Save(urlStr, StatusTimeout) - } + // TODO: Will be controlled by CacheAllExternal option + // if !hT.opts.RetryCachedErrors { + // hT.refCache.Save(urlStr, StatusTimeout) + // } hT.issueStore.AddIssue(issues.Issue{ Level: issueLevel, Message: "request exceeded our ExternalTimeout", From bb3ec96dc2fc5c228c37325935328cd4dc5e4f9b Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 06:39:55 -0400 Subject: [PATCH 04/14] Phase 0 done; test Options, and first use of assert lib --- .cspell.yml | 1 + docs/tasks/cache-unchecked-external-links.md | 119 +++++++++++++------ go.mod | 1 + go.sum | 17 +++ htmltest/check-link-cache_test.go | 42 ++++--- htmltest/check-link.go | 7 +- htmltest/options.go | 26 ++-- 7 files changed, 145 insertions(+), 68 deletions(-) diff --git a/.cspell.yml b/.cspell.yml index c491cfc..5d8e78d 100644 --- a/.cspell.yml +++ b/.cspell.yml @@ -33,4 +33,5 @@ words: - regexs - rels - seborama + - stretchr - wjdp diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index abcd242..0003497 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -5,6 +5,14 @@ lastmod: 2025-10-19 status: in-progress --- +## Status + +- ✅ **Phase 0 Complete**: `RetryCachedErrors` semantics cleaned up, timeout + caching now controlled by `CacheAllExternal` +- ✅ **Test Infrastructure**: Migrated `check-link-cache_test.go` to use + `testify/assert` for better assertion syntax +- 🚧 **Phase 1 In Progress**: Ready to implement Discovery Mode feature + # CacheAllExternal Feature ## Use Case @@ -62,16 +70,16 @@ This cleanup must happen BEFORE implementing the discovery mode feature. ### Complete Behavior Matrix -| CheckExternal | CacheAllExternal | RetryCachedErrors | Links Checked? | Errors Retried? | What Gets Cached | Use Case | -| ------------- | ---------------- | ----------------- | -------------- | --------------- | ----------------- | ------------------------------------ | -| `true` | `false` | `true` | ✓ | ✓ | 200, 4XX | **Default/Legacy** | -| `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Failed links are not retried | -| `true` | `true` | `true` | ✓ | ✓ | 200, 4XX, TSC[^1] | Cache timeouts, but retry | -| `true` | `true` | `false` | ✓ | ✗ | 200, 4XX, TSC[^1] | **Fast re-runs** - cache & reuse all | -| `false` | `false` | (N/A) | ✗ | N/A | Nothing | **Default skip** - no cache | -| `false` | `true` | (N/A) | ✗ | N/A | unchecked links | **Link discovery** | +| CheckExternal | CacheAllExternal | RetryCachedErrors | Links Checked? | Errors Retried? | What Gets Cached | Use Case | +| ------------- | ---------------- | ----------------- | -------------- | --------------- | ------------------- | ------------------------------------ | +| `true` | `false` | `true` | ✓ | ✓ | 200, 4XX | **Default/Legacy** | +| `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Failed links are not retried | +| `true` | `true` | `true` | ✓ | ✓ | 200, 4XX, TSC[^TSC] | Cache timeouts, but retry | +| `true` | `true` | `false` | ✓ | ✗ | 200, 4XX, TSC[^TSC] | **Fast re-runs** - cache & reuse all | +| `false` | `false` | (N/A) | ✗ | N/A | Nothing | **Default skip** - no cache | +| `false` | `true` | (N/A) | ✗ | N/A | unchecked links | **Link discovery** | -[^1]: +[^TSC]: TSC = Tool-specific status code used by htmltest. See `@docs/tasks/design.md` for details. @@ -102,17 +110,28 @@ For each behavior in the matrix below: ### Test Order & Behaviors -| # | Behavior to Test | Config | Expected Result | Test Name | -| --- | ------------------- | -------------------------------------------------- | -------------------------------- | --------------------------------- | -| 1 | Default: no caching | `CheckExternal: false`, `CacheAllExternal: false` | Nothing cached | `TestCacheAllExternalDisabled` | -| 2 | **Discovery mode** | `CheckExternal: false`, `CacheAllExternal: true` | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | -| 3 | **Timeout caching** | `CheckExternal: true`, `CacheAllExternal: true` | Cache timeout as `StatusTimeout` | `TestCacheAllExternalTimeout` | -| 4 | Ignored URLs | `CacheAllExternal: true`, `IgnoreURLs: [pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalIgnored` | -| 5 | Query stripping | `CacheAllExternal: true`, `StripQueryString: true` | Query stripped | `TestCacheAllExternalQueryString` | +Phase 1 focuses on implementing **Discovery Mode** (rows 5-6 from Complete +Behavior Matrix). Timeout caching (rows 3-4) was already implemented in Phase 0. + +| # | Matrix Row | Behavior to Test | CheckExternal-related config[^Config] | Expected Result | Test Name | Status | +| --- | ---------- | --------------------------- | ------------------------------------------------ | ---------------------------- | ------------------------------------- | ---------- | +| 1 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Exists | +| 2 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | TODO | +| 3 | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout retried on next run | _(covered by existing timeout tests)_ | ✅ Phase 0 | +| 4 | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout reused from cache | `TestTimeoutCachedReused` | ✅ Phase 0 | +| 5 | — | Ignored URLs (edge case) | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalIgnored` | TODO | +| 6 | — | Query stripping (edge case) | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | TODO | + +[^Config]: + Abbreviations: `Check` = `CheckExternal`, `All` = `CacheAllExternal`, + `Retry` = `RetryCachedErrors`. **Commands**: Use `make test-tdd TEST_RUN=` or `make test-tdd-cache TEST_RUN=` +**Note**: Phase 1 focuses on discovery mode (row 6) and its edge cases. Rows 1-4 +from the Complete Behavior Matrix are already covered by existing tests. + ### 1. Write Tests (One at a Time) **File**: `htmltest/check-link-cache_test.go` @@ -184,39 +203,63 @@ Two modifications needed: ## Incremental Implementation Strategy -### Phase 0: Cleanup `RetryCachedErrors` Semantics +### Phase 0: Cleanup `RetryCachedErrors` Semantics ✅ COMPLETE **Purpose**: Separate concerns before adding new functionality -#### Step 0a: Add config option +**Step 0.1**: Write test for new semantics + +- ✅ Created `TestTimeoutNotCachedWithRetryCacheErrorsOnly` - verifies timeouts + are NOT cached when only `RetryCachedErrors: false` (without + `CacheAllExternal`) +- ✅ Test run: **GREEN** (existing code already has correct behavior for this + case) + +**Step 0.2**: Update existing test to expect new behavior (RED) + +- ✅ Updated `TestTimeoutIsCached` to use `CacheAllExternal: true` instead of + `RetryCachedErrors: false` +- ✅ Test cannot run yet (references non-existent option) + +**Step 0.3**: Change implementation to use new option (RED) + +- ✅ Changed `check-link.go` timeout caching condition from + `if !hT.opts.RetryCachedErrors` to `if hT.opts.CacheAllExternal` +- ✅ Result: **Compilation error** - option doesn't exist yet + +**Step 0.4**: Add option to make code compile (GREEN) + +- ✅ Added `CacheAllExternal bool` to `Options` struct (`htmltest/options.go`) +- ✅ Added default value `"CacheAllExternal": false` in `DefaultOptions()` +- ✅ Test run: **GREEN** - code compiles and test passes -- Add `CacheAllExternal bool` to `Options` struct -- Add default value `false` in `DefaultOptions()` -- Run existing tests: Should PASS (option exists but unused) +**Step 0.5**: Update remaining timeout tests -#### Step 0b: Update existing timeout tests +- ✅ Updated `TestTimeoutCachedReused` to use `CacheAllExternal: true` +- ✅ Updated `TestTimeoutCachedMessage` to use `CacheAllExternal: true` +- ✅ All cache tests: **GREEN** -- Update 3 existing tests to use `CacheAllExternal: true` instead of - `RetryCachedErrors: false`: - - `TestTimeoutIsCached` - - `TestTimeoutCachedReused` - - `TestTimeoutCachedMessage` -- Run tests: RED (tests expect new behavior, code uses old) +**Step 0.6**: Add sanity check test -#### Step 0c: Refactor timeout caching code +- ✅ Created `TestCacheOptions` to verify default option values +- ✅ Test run: **GREEN** +- ✅ Full test suite: All packages pass -- In `check-link.go`, change timeout caching condition from: - - OLD: `if !hT.opts.RetryCachedErrors` - - NEW: `if hT.opts.CacheAllExternal` -- Run tests: GREEN (existing tests verify behavior) +**Result**: Clean semantics established, timeout caching now controlled by +`CacheAllExternal` (not `RetryCachedErrors`), all tests passing, no regression -#### Step 0d: Verify row 2 behavior +**Key TDD Insight**: We changed the implementation BEFORE the option existed, +causing a compilation error (deep RED), then added just enough to compile and +pass (GREEN) -- Ensure `TestTimeoutNotCachedByDefault` still passes -- This test verifies row 2: `CacheAllExternal: false` + - `RetryCachedErrors: false` = NO timeout caching +**Infrastructure Improvement**: Testify migration -**Benefit**: Clean semantics established, existing tests ensure no regression +- ✅ Migrated `check-link-cache_test.go` from `go-assert` to + `github.com/stretchr/testify/assert` +- ✅ Benefits: Industry-standard library, better error messages, active + maintenance +- ✅ All existing tests continue to pass +- ✅ New tests use cleaner assertion syntax: `assert.Equal(t, expected, actual)` --- diff --git a/go.mod b/go.mod index 15ccfb8..17cae07 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/golangplus/sort v1.0.0 // indirect github.com/imdario/mergo v0.3.11 github.com/seborama/govcr v4.5.0+incompatible // indirect + github.com/stretchr/testify v1.11.1 // indirect golang.org/x/net v0.17.0 gopkg.in/seborama/govcr.v4 v4.5.0 gopkg.in/yaml.v2 v2.4.0 diff --git a/go.sum b/go.sum index 91103b3..c3caa80 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,8 @@ github.com/badoux/checkmail v1.2.1 h1:TzwYx5pnsV6anJweMx2auXdekBwGr/yt1GgalIx9nBQ= github.com/badoux/checkmail v1.2.1/go.mod h1:XroCOBU5zzZJcLvgwU15I+2xXyCdTWXyR9MGfRhBYy0= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/daviddengcn/go-algs v0.0.0-20180330170136-fe23fabd9d06 h1:jEHTltplMBbYsHlSnvZD1J4CsfweUSJIqS8uP56q1Ng= github.com/daviddengcn/go-algs v0.0.0-20180330170136-fe23fabd9d06/go.mod h1:CpyLopUWBmqupyWU6OlSfrzgIuzNq0R6DXzM74O9RMs= github.com/daviddengcn/go-assert v0.0.0-20150305222929-ba7e68aeeff6 h1:OPIYL/VhQiSpoaxIcmeYdghLswBylfk6JDVCUqadXxg= @@ -25,8 +28,19 @@ github.com/mattn/go-colorable v0.1.8 h1:c1ghPdyEDarC70ftn0y+A/Ee++9zz8ljHG1b13eJ github.com/mattn/go-colorable v0.1.8/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= github.com/mattn/go-isatty v0.0.12 h1:wuysRhFDzyxgEmMf5xjvJ2M9dZoWAXNNr5LSBS7uHXY= github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/seborama/govcr v4.5.0+incompatible h1:XvdHtXi0d4cUAn+0aWolvwfS3nmhNC8Z+yMQwn/M64I= github.com/seborama/govcr v4.5.0+incompatible/go.mod h1:EgcISudCCYDLzbiAImJ8i7kk4+wTA44Kp+j4S0LhASI= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= @@ -77,3 +91,6 @@ gopkg.in/seborama/govcr.v4 v4.5.0/go.mod h1:GSulKzJ4LIk6XH6sE+OKL76FwDIVQEja2Oh2 gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 6e91c42..18b5a0d 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -3,6 +3,7 @@ package htmltest import ( "testing" + "github.com/stretchr/testify/assert" "github.com/wjdp/htmltest/issues" ) @@ -10,6 +11,18 @@ import ( // Feature: RetryCachedErrors // Added by @chalin +func TestCacheOptions(t *testing.T) { + opts := DefaultOptions() + + // Sanity check: ensure that non-existent option doesn't exist + _, exists := opts["NonExistentOption"] + assert.False(t, exists, "NonExistentOption should not exist") + + // Verify cache-related option defaults + assert.Equal(t, false, opts["CacheAllExternal"], "CacheAllExternal default") + assert.Equal(t, true, opts["RetryCachedErrors"], "RetryCachedErrors default") +} + // ======================================== // External Error Caching Tests // ======================================== @@ -103,9 +116,9 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { } } -// TestTimeoutNotCachedWithRetryCacheErrorsOnly : Test that timeouts are NOT cached -// when ONLY RetryCachedErrors is false (without CacheAllExternal). -// This verifies the cleanup: RetryCachedErrors no longer controls timeout caching. +// TestTimeoutNotCachedWithRetryCacheErrorsOnly : Test that timeouts are NOT +// cached when ONLY RetryCachedErrors is false (without CacheAllExternal). This +// ensures that the legacy behavior of RetryCachedErrors is no longer active. func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { tSkipShortExternal(t) @@ -124,11 +137,11 @@ func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { } // TestTimeoutIsCached : Test that URLs that timeout are saved to the refcache -// when RetryCachedErrors is false. +// when CacheAllExternal is true. func TestTimeoutIsCached(t *testing.T) { tSkipShortExternal(t) hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) + map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true}) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) @@ -143,19 +156,19 @@ func TestTimeoutIsCached(t *testing.T) { } // TestTimeoutCachedReused : Test that cached timeout results are reused on -// subsequent runs without retrying the URL when RetryCachedErrors is false and -// CacheAllExternal is true. TODO: fix me. +// subsequent runs without retrying the URL when CacheAllExternal is true and +// RetryCachedErrors is false. func TestTimeoutCachedReused(t *testing.T) { tSkipShortExternal(t) // First run: cause and cache a timeout hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) + map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 1) - // Second run: WITHOUT timeout set - should use cached 524, not actually try the request + // Second run: WITHOUT timeout set - should use cached timeout, not actually try the request hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", - map[string]interface{}{"EnableCache": true, "RetryCachedErrors": false, "LogLevel": issues.LevelDebug}) + map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false, "LogLevel": issues.LevelDebug}) tExpectIssueCount(t, hT2, 1) // Verify it used the cache (should see "from cache" not "hitting") @@ -163,23 +176,24 @@ func TestTimeoutCachedReused(t *testing.T) { t.Error("expected cached timeout to be reused (should see 'from cache' message)") } if hT2.issueStore.MessageMatchCount("hitting") > 0 { - t.Error("should not retry when timeout is cached and RetryCachedErrors is false") + t.Error("should not retry when timeout is cached (CacheAllExternal=true, RetryCachedErrors=false)") } } // TestTimeoutCachedMessage : Test that a URL that previously timed out will be -// reported as "(cached)" on subsequent runs. when RetryCachedErrors is false. +// reported as "(cached)" on subsequent runs when CacheAllExternal is true and +// RetryCachedErrors is false. func TestTimeoutCachedMessage(t *testing.T) { tSkipShortExternal(t) // First run: cause and cache a timeout hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) + map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT, 1) // Second run: with RetryCachedErrors disabled, should use cached timeout and report with "(cached)" message hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", - map[string]interface{}{"EnableCache": true, "RetryCachedErrors": false}) + map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) tExpectIssueCount(t, hT2, 1) tExpectIssue(t, hT2, "request exceeded our ExternalTimeout (cached)", 1) } diff --git a/htmltest/check-link.go b/htmltest/check-link.go index d26a4f5..58c590d 100644 --- a/htmltest/check-link.go +++ b/htmltest/check-link.go @@ -199,10 +199,9 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { if err != nil { if strings.Contains(err.Error(), "Client.Timeout") { - // TODO: Will be controlled by CacheAllExternal option - // if !hT.opts.RetryCachedErrors { - // hT.refCache.Save(urlStr, StatusTimeout) - // } + if hT.opts.CacheAllExternal { + hT.refCache.Save(urlStr, StatusTimeout) + } hT.issueStore.AddIssue(issues.Issue{ Level: issueLevel, Message: "request exceeded our ExternalTimeout", diff --git a/htmltest/options.go b/htmltest/options.go index 09241ba..39282c2 100644 --- a/htmltest/options.go +++ b/htmltest/options.go @@ -68,12 +68,13 @@ type Options struct { StripQueryString bool StripQueryExcludes []interface{} - EnableCache bool - EnableLog bool - OutputDir string - OutputCacheFile string - OutputLogFile string - CacheExpires string // Accepts golang time period strings, hours (16h) is really only useful option + EnableCache bool + CacheAllExternal bool + EnableLog bool + OutputDir string + OutputCacheFile string + OutputLogFile string + CacheExpires string // Accepts golang time period strings, hours (16h) is really only useful option // --- Internals below here --- NoRun bool // When true does not run tests, used to inspect state in unit tests @@ -140,12 +141,13 @@ func DefaultOptions() map[string]interface{} { "StripQueryString": true, "StripQueryExcludes": []interface{}{"fonts.googleapis.com"}, - "EnableCache": true, - "EnableLog": true, - "OutputDir": path.Join("tmp", ".htmltest"), - "OutputCacheFile": "refcache.json", - "OutputLogFile": "htmltest.log", - "CacheExpires": "336h", + "EnableCache": true, + "CacheAllExternal": false, + "EnableLog": true, + "OutputDir": path.Join("tmp", ".htmltest"), + "OutputCacheFile": "refcache.json", + "OutputLogFile": "htmltest.log", + "CacheExpires": "336h", "NoRun": false, "VCREnable": false, From 6180775362f5b57c43b9ce296ce15adb464b6d18 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 06:55:34 -0400 Subject: [PATCH 05/14] New test helpers --- htmltest/check-link-cache_test.go | 5 +---- htmltest/test_helpers_extra_test.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 18b5a0d..3afffd5 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -212,8 +212,5 @@ func TestCacheAllExternalDisabled(t *testing.T) { tExpectIssueCount(t, hT, 0) // No errors since external checking is disabled // Verify the external link was NOT cached (default behavior) - _, ok := hT.refCache.Get("http://www.asdo3IRJ395295jsingrkrg4.com") - if ok { - t.Error("external links should NOT be cached when CacheAllExternal is false (default)") - } + tExpectNotCached(t, hT, "http://www.asdo3IRJ395295jsingrkrg4.com") } diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go index 0c7dfbf..2efa8a8 100644 --- a/htmltest/test_helpers_extra_test.go +++ b/htmltest/test_helpers_extra_test.go @@ -2,8 +2,10 @@ package htmltest import ( "os" + "testing" "github.com/imdario/mergo" + "github.com/stretchr/testify/assert" ) // Test fixture helpers @@ -21,3 +23,21 @@ func tRemoveOutputDir(tOpts map[string]interface{}) { mergo.MergeWithOverwrite(&opts, tOpts) os.RemoveAll(opts["OutputDir"].(string)) } + +// Cache assertion helpers + +// tExpectCached asserts that a URL is present in the refcache. +// If statusCode is provided, also asserts the cached status code matches. +func tExpectCached(t *testing.T, hT *HTMLTest, url string, statusCode ...int) { + cR, ok := hT.refCache.Get(url) + assert.True(t, ok, "URL should be cached: "+url) + if len(statusCode) > 0 { + assert.Equal(t, statusCode[0], cR.StatusCode, "cached status code") + } +} + +// tExpectNotCached asserts that a URL is NOT present in the refcache. +func tExpectNotCached(t *testing.T, hT *HTMLTest, url string) { + _, ok := hT.refCache.Get(url) + assert.False(t, ok, "URL should not be cached: "+url) +} From 69f2a93129f15fbc26c1e84d30c389f21d8493ad Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 07:45:18 -0400 Subject: [PATCH 06/14] Some test refactoring to use helpers --- htmltest/check-link-cache_test.go | 130 +++++++++++------------------- 1 file changed, 49 insertions(+), 81 deletions(-) diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 3afffd5..a9e7bce 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/wjdp/htmltest/issues" ) // Tests for cache-related functionality @@ -33,58 +32,46 @@ func TestExternalErrorCached(t *testing.T) { hT := tTestFileOptsFromCleanOutputDir("fixtures/images/imageExternal404.html", map[string]interface{}{"VCREnable": true, "EnableCache": true}) tExpectIssueCount(t, hT, 1) - - // Verify the 404 was saved to cache - cR, ok := hT.refCache.Get("https://upload.wikimedia.org/wikipedia/en/404") - if !ok { - t.Error("expected 404 response to be cached, but it wasn't") - } - // Verify it's actually a 404 - if cR.StatusCode != 404 { - t.Errorf("expected status code 404 in cache, got %d", cR.StatusCode) - } + tExpectCached(t, hT, "https://upload.wikimedia.org/wikipedia/en/404", 404) } // TestExternalErrorCachedRetried : Test that by default cached error responses // (404, etc.) are retried on subsequent runs. func TestExternalErrorCachedRetried(t *testing.T) { + fixture := "fixtures/images/imageExternal404.html" + opts := map[string]interface{}{"VCREnable": true, "EnableCache": true} + // First run: populate cache with 404 - hT := tTestFileOptsFromCleanOutputDir("fixtures/images/imageExternal404.html", - map[string]interface{}{"VCREnable": true, "EnableCache": true}) + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) tExpectIssueCount(t, hT, 1) // Second run: WITH VCR - should retry even though 404 is cached (default behavior) - hT2 := tTestFileOpts("fixtures/images/imageExternal404.html", - map[string]interface{}{"VCREnable": true, "EnableCache": true, "LogLevel": issues.LevelDebug}) + hT2 := tTestFileOpts(fixture, opts) tExpectIssueCount(t, hT2, 1) // Verify it did NOT use the cache (should see "fresh" not "from cache") - if hT2.issueStore.MessageMatchCount("from cache") > 0 { - t.Error("expected cached 404 to be retried (should NOT see 'from cache' message by default)") - } - if hT2.issueStore.MessageMatchCount("fresh") == 0 { - t.Error("expected cached 404 to be retried (should see 'fresh' message)") - } + tExpectIssue(t, hT2, "from cache", 0) + tExpectIssue(t, hT2, "fresh", 1) } // TestExternalBrokenRetryCachedErrorsDisabled : Test that URLs with non-OK -// status are not retried when RetryCachedErrors is set to false. -// Uses elements for variety. +// status are not retried when RetryCachedErrors is false. func TestExternalBrokenRetryCachedErrorsDisabled(t *testing.T) { - // First run: populate cache with 404 - hT := tTestFileOptsFromCleanOutputDir("fixtures/generic/citeBroken.html", - map[string]interface{}{"VCREnable": true, "EnableCache": true, "RetryCachedErrors": false}) - tExpectIssueCount(t, hT, 4) // 4 broken citations in the fixture - - // Second run: WITHOUT VCR - should use cached 404s, not retry (which would fail without VCR) - hT2 := tTestFileOpts("fixtures/generic/citeBroken.html", - map[string]interface{}{"EnableCache": true, "RetryCachedErrors": false, "LogLevel": issues.LevelDebug}) - tExpectIssueCount(t, hT2, 4) - - // Verify it used the cache by checking for "from cache" messages - if hT2.issueStore.MessageMatchCount("from cache") == 0 { - t.Error("expected cached 404s to be reused (should see 'from cache' messages)") - } + fixture := "fixtures/images/imageExternal404.html" + opts := map[string]interface{}{"VCREnable": true, "EnableCache": true, "RetryCachedErrors": false} + + // First run WITH VCR: populate cache with 404 + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 1) + tExpectCached(t, hT, "https://upload.wikimedia.org/wikipedia/en/404", 404) + + // Second run: should use cached 404 (not retry because RetryCachedErrors is false) + hT2 := tTestFileOpts(fixture, opts) + tExpectIssueCount(t, hT2, 1) + + // Verify it used the cache (not retried) + tExpectIssue(t, hT2, "from cache", 1) + tExpectIssue(t, hT2, "hitting", 0) } // ======================================== @@ -95,89 +82,70 @@ func TestExternalBrokenRetryCachedErrorsDisabled(t *testing.T) { // and are retried on every run. This ensures backward compatibility with the // original behavior. func TestTimeoutNotCachedByDefault(t *testing.T) { + fixture := "fixtures/links/ip_timeout.html" tSkipShortExternal(t) // First run: timeout occurs - opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "LogLevel": issues.LevelDebug} - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true} + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) // Second run: should retry (not use cache) because RetryCachedErrors is true (default) - hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", opts) + hT2 := tTestFileOpts(fixture, opts) tExpectIssueCount(t, hT2, 1) // Verify it retried (should see "fresh" and "hitting", not "from cache") - if hT2.issueStore.MessageMatchCount("fresh") == 0 { - t.Error("timeout should be retried by default (should see 'fresh' message)") - } - if hT2.issueStore.MessageMatchCount("hitting") == 0 { - t.Error("timeout should be retried by default (should see 'hitting' message)") - } + tExpectIssue(t, hT2, "fresh", 1) + tExpectIssue(t, hT2, "hitting", 1) + tExpectIssue(t, hT2, "from cache", 0) } // TestTimeoutNotCachedWithRetryCacheErrorsOnly : Test that timeouts are NOT // cached when ONLY RetryCachedErrors is false (without CacheAllExternal). This // ensures that the legacy behavior of RetryCachedErrors is no longer active. func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false} tSkipShortExternal(t) - - // Run with RetryCachedErrors: false but CacheAllExternal: false (default) - // After cleanup, this should NOT cache timeouts - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false}) + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) - // Verify the timeout was NOT cached (new behavior after cleanup) - _, ok := hT.refCache.Get("http://5.6.7.8") - if ok { - t.Error("timeout should NOT be cached when only RetryCachedErrors is false (without CacheAllExternal)") - } + tExpectNotCached(t, hT, "http://5.6.7.8") } // TestTimeoutIsCached : Test that URLs that timeout are saved to the refcache // when CacheAllExternal is true. func TestTimeoutIsCached(t *testing.T) { tSkipShortExternal(t) - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true}) + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true} + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) - - // Verify the timeout was saved to cache with StatusTimeout - cR, ok := hT.refCache.Get("http://5.6.7.8") - if !ok { - t.Error("expected timeout to be cached when RetryCachedErrors is false, but it wasn't") - } - if cR.StatusCode != StatusTimeout { - t.Errorf("expected status code %d (StatusTimeout), got %d", StatusTimeout, cR.StatusCode) - } + tExpectCached(t, hT, "http://5.6.7.8", StatusTimeout) } // TestTimeoutCachedReused : Test that cached timeout results are reused on // subsequent runs without retrying the URL when CacheAllExternal is true and // RetryCachedErrors is false. func TestTimeoutCachedReused(t *testing.T) { + fixture := "fixtures/links/ip_timeout.html" + opts := map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false} tSkipShortExternal(t) // First run: cause and cache a timeout - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) + opts["ExternalTimeout"] = 1 + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) tExpectIssueCount(t, hT, 1) // Second run: WITHOUT timeout set - should use cached timeout, not actually try the request - hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", - map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false, "LogLevel": issues.LevelDebug}) + delete(opts, "ExternalTimeout") + hT2 := tTestFileOpts(fixture, opts) tExpectIssueCount(t, hT2, 1) // Verify it used the cache (should see "from cache" not "hitting") - if hT2.issueStore.MessageMatchCount("from cache") == 0 { - t.Error("expected cached timeout to be reused (should see 'from cache' message)") - } - if hT2.issueStore.MessageMatchCount("hitting") > 0 { - t.Error("should not retry when timeout is cached (CacheAllExternal=true, RetryCachedErrors=false)") - } + tExpectIssue(t, hT2, "from cache", 1) + tExpectIssue(t, hT2, "hitting", 0) } // TestTimeoutCachedMessage : Test that a URL that previously timed out will be @@ -185,15 +153,15 @@ func TestTimeoutCachedReused(t *testing.T) { // RetryCachedErrors is false. func TestTimeoutCachedMessage(t *testing.T) { tSkipShortExternal(t) + fixture := "fixtures/links/ip_timeout.html" + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false} // First run: cause and cache a timeout - hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", - map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) tExpectIssueCount(t, hT, 1) // Second run: with RetryCachedErrors disabled, should use cached timeout and report with "(cached)" message - hT2 := tTestFileOpts("fixtures/links/ip_timeout.html", - map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false}) + hT2 := tTestFileOpts(fixture, opts) tExpectIssueCount(t, hT2, 1) tExpectIssue(t, hT2, "request exceeded our ExternalTimeout (cached)", 1) } From 3694b3fa55c77a884d07270224be2c74a3f8cb5f Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 08:18:53 -0400 Subject: [PATCH 07/14] Add support for skipping slow tests via flag, and list tool-errors --- Makefile | 18 ++-- docs/tasks/cache-unchecked-external-links.md | 105 +++++++++++++++++-- htmltest/check-link-cache_test.go | 28 ++--- htmltest/statuscodes.go | 4 + htmltest/test_helpers_extra_test.go | 14 +++ 5 files changed, 131 insertions(+), 38 deletions(-) diff --git a/Makefile b/Makefile index 2b004b7..234ace0 100644 --- a/Makefile +++ b/Makefile @@ -48,17 +48,17 @@ build-verify: build ## test: Run all tests (use TESTFLAGS to pass additional flags, e.g., make test TESTFLAGS="-v") test: - @echo "Running tests..." + @echo "Running ALL tests (includes slow and external tests)..." @go test $(TESTFLAGS) ./... ## test-race: Run tests with race detector (recommended) test-race: - @echo "Running tests with race detector..." + @echo "Running tests with race detector (includes slow and external tests)..." @go test -v -race ./... ## test-coverage: Generate and display test coverage test-coverage: - @echo "Generating coverage report..." + @echo "Generating coverage report (includes slow and external tests)..." @go test $(TESTFLAGS) -coverprofile=coverage.txt ./... @go tool cover -func=coverage.txt @echo "" @@ -84,16 +84,16 @@ clean-cache: ## test-tdd: TDD mode - run specific test(s) with clean cache (use TEST_RUN to match tests). ## Examples: -## make test-tdd TEST_RUN=TestTimeoutIsCached # Run single test with clean cache -## make test-tdd TEST_RUN='.*Cache.*' # Run all cache tests -## make test-tdd TEST_RUN=TestTimeout # Run tests matching pattern -## make test-clean TEST_RUN=TestTimeout # Same but without cache inspection -## make clean-cache # Just clean the cache +## make test-tdd TEST_RUN=TestTimeoutIsCached # Run single test +## make test-tdd TEST_RUN='.*Cache.*' # Run all cache tests +## make test-tdd TEST_RUN='.*Cache.*' TESTFLAGS=-skip-slow # Skip slow tests (timeouts) +## make test-tdd-cache TEST_RUN=TestTimeout # Same with cache inspection +## make clean-cache # Just clean the cache test-tdd: clean-cache _test-tdd _test-tdd: @echo "TDD mode: Running $(TEST_RUN) in $(TEST_PKG)..." - @go test -v -run $(TEST_RUN) $(TEST_PKG) || true + @go test -v -run $(TEST_RUN) $(TEST_PKG) $(TESTFLAGS) || true @echo "" @echo "Test completed." diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index 0003497..548cb60 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -86,7 +86,8 @@ This cleanup must happen BEFORE implementing the discovery mode feature. ### Key Insights - `CacheAllExternal` extends caching behavior in **both** modes: - - When `CheckExternal: true` → Also caches timeouts (as `StatusTimeout`) + - When `CheckExternal: true` → Caches **all tool-specific errors** (timeouts, + network failures, etc.) - When `CheckExternal: false` → Caches discovered links (as `StatusUnchecked`) - `RetryCachedErrors` now has clean, single-purpose semantics (retry vs. reuse cached errors) @@ -94,6 +95,33 @@ This cleanup must happen BEFORE implementing the discovery mode feature. retry) - See `@docs/tasks/design.md` for status code details +### Tool-Specific Errors to Cache + +When `CacheAllExternal: true` and `CheckExternal: true`, **all** non-HTTP errors +should be cached with appropriate status codes: + +1. **Timeout errors** (`StatusTimeout = -10`): + - Request exceeds `ExternalTimeout` setting + - Currently: ✅ Already implemented in Phase 0 + +2. **DNS/Network errors** (`StatusNetworkError = -20`): + - "dial tcp" failures + - DNS lookup failures + - Connection refused + - Currently: ❌ Not cached (returns early without caching) + +3. **Certificate errors** (`StatusCertError = -30`): + - x509.UnknownAuthorityError + - Invalid/expired certificates + - Incomplete certificate chains + - Currently: ❌ Not cached (returns early without caching) + +4. **Generic client errors** (`StatusClientError = -40`): + - Other unhandled HTTP client errors + - Currently: ❌ Not cached (returns early without caching) + +**Note**: HTTP status codes (200, 404, etc.) are already cached in all modes. + ## Implementation Steps (TDD Approach) **Incremental TDD**: Write one test at a time, implement, verify, then move to @@ -252,19 +280,43 @@ Two modifications needed: causing a compilation error (deep RED), then added just enough to compile and pass (GREEN) -**Infrastructure Improvement**: Testify migration - -- ✅ Migrated `check-link-cache_test.go` from `go-assert` to - `github.com/stretchr/testify/assert` -- ✅ Benefits: Industry-standard library, better error messages, active - maintenance -- ✅ All existing tests continue to pass -- ✅ New tests use cleaner assertion syntax: `assert.Equal(t, expected, actual)` +**Infrastructure Improvements**: + +1. **Testify migration**: + - ✅ Migrated `check-link-cache_test.go` to + `github.com/stretchr/testify/assert` + - ✅ Benefits: Industry-standard library, better error messages, active + maintenance + - ✅ Cleaner assertion syntax: `assert.Equal(t, expected, actual)` + +2. **Cache assertion helpers** (`test_helpers_extra_test.go`): + - ✅ `tExpectCached(t, hT, url, statusCode...)` - Assert URL is cached + (optionally with status) + - ✅ `tExpectNotCached(t, hT, url)` - Assert URL is not cached + - ✅ Reduces boilerplate from ~7 lines to 1 line per assertion + +3. **Slow test control**: + - ✅ Added `tSkipSlow(t)` helper with `-skip-slow` flag + - ✅ Applied to 4 timeout tests (wait 1s each) + - ✅ Updated Makefile to support `TESTFLAGS` + - ✅ Usage: `make test-tdd TEST_RUN='.*Cache.*' TESTFLAGS=-skip-slow` + - ✅ Result: Cache tests run in ~1.7s (vs ~7.8s with timeout waits) + +4. **Test refactoring** (DRY patterns): + - ✅ Extract `fixture` variables for paths + - ✅ Extract `opts` maps and reuse across test runs + - ✅ Consistent test structure across all cache tests --- ### Phase 1: Discovery Mode Feature +**Scope**: Implement `StatusUnchecked` caching when `CheckExternal: false` and +`CacheAllExternal: true`. + +**Note**: Phase 1 focuses on discovery mode only. Caching additional tool-specific +errors (network failures, cert errors) will be addressed in future phases. + #### Increment 1: Test #1 - Default Behavior (Baseline) **Purpose**: Establish regression test for row 5 @@ -334,3 +386,38 @@ fresh, verify complete matrix coverage. ### Documentation - [ ] Update README configuration table + +## Future Phases + +### Phase 2: Network Error Caching (Future) + +**Scope**: Cache DNS and network failures with `StatusNetworkError = -20` + +- Add `StatusNetworkError` constant to `statuscodes.go` +- Modify "dial tcp" error handling in `check-link.go` to cache when + `CacheAllExternal: true` +- Add tests for network error caching +- Covers: DNS lookup failures, connection refused, network unreachable + +### Phase 3: Certificate Error Caching (Future) + +**Scope**: Cache certificate validation errors with `StatusCertError = -30` + +- Add `StatusCertError` constant to `statuscodes.go` +- Modify x509 error handling in `check-link.go` to cache when `CacheAllExternal: + true` +- Add tests for certificate error caching +- Covers: Unknown authority, expired certs, incomplete chains + +### Phase 4: Generic Client Error Caching (Future) + +**Scope**: Cache other HTTP client errors with `StatusClientError = -40` + +- Add `StatusClientError` constant to `statuscodes.go` +- Modify generic error handling in `check-link.go` to cache when + `CacheAllExternal: true` +- Add tests for generic error caching +- Covers: All other unhandled HTTP client errors + +**Note**: These phases follow the same TDD approach as Phases 0 and 1. Each error +type gets its own status code and test coverage. diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index a9e7bce..0d37107 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -82,8 +82,9 @@ func TestExternalBrokenRetryCachedErrorsDisabled(t *testing.T) { // and are retried on every run. This ensures backward compatibility with the // original behavior. func TestTimeoutNotCachedByDefault(t *testing.T) { - fixture := "fixtures/links/ip_timeout.html" + tSkipSlow(t) tSkipShortExternal(t) + fixture := "fixtures/links/ip_timeout.html" // First run: timeout occurs opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true} @@ -105,8 +106,10 @@ func TestTimeoutNotCachedByDefault(t *testing.T) { // cached when ONLY RetryCachedErrors is false (without CacheAllExternal). This // ensures that the legacy behavior of RetryCachedErrors is no longer active. func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { - opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false} + tSkipSlow(t) tSkipShortExternal(t) + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "RetryCachedErrors": false} + hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) @@ -117,6 +120,7 @@ func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { // TestTimeoutIsCached : Test that URLs that timeout are saved to the refcache // when CacheAllExternal is true. func TestTimeoutIsCached(t *testing.T) { + tSkipSlow(t) tSkipShortExternal(t) opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true} hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) @@ -129,9 +133,10 @@ func TestTimeoutIsCached(t *testing.T) { // subsequent runs without retrying the URL when CacheAllExternal is true and // RetryCachedErrors is false. func TestTimeoutCachedReused(t *testing.T) { + tSkipSlow(t) + tSkipShortExternal(t) fixture := "fixtures/links/ip_timeout.html" opts := map[string]interface{}{"EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false} - tSkipShortExternal(t) // First run: cause and cache a timeout opts["ExternalTimeout"] = 1 @@ -146,23 +151,6 @@ func TestTimeoutCachedReused(t *testing.T) { // Verify it used the cache (should see "from cache" not "hitting") tExpectIssue(t, hT2, "from cache", 1) tExpectIssue(t, hT2, "hitting", 0) -} - -// TestTimeoutCachedMessage : Test that a URL that previously timed out will be -// reported as "(cached)" on subsequent runs when CacheAllExternal is true and -// RetryCachedErrors is false. -func TestTimeoutCachedMessage(t *testing.T) { - tSkipShortExternal(t) - fixture := "fixtures/links/ip_timeout.html" - opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": false} - - // First run: cause and cache a timeout - hT := tTestFileOptsFromCleanOutputDir(fixture, opts) - tExpectIssueCount(t, hT, 1) - - // Second run: with RetryCachedErrors disabled, should use cached timeout and report with "(cached)" message - hT2 := tTestFileOpts(fixture, opts) - tExpectIssueCount(t, hT2, 1) tExpectIssue(t, hT2, "request exceeded our ExternalTimeout (cached)", 1) } diff --git a/htmltest/statuscodes.go b/htmltest/statuscodes.go index 40411c3..516f6c7 100644 --- a/htmltest/statuscodes.go +++ b/htmltest/statuscodes.go @@ -13,6 +13,10 @@ For details, see @docs/tasks/migrate-status-codes.md const ( StatusUnchecked = 0 StatusTimeout = -10 + // Future: Additional tool-specific error codes (not yet implemented) + // StatusNetworkError = -20 // DNS failures, connection refused, etc. + // StatusCertError = -30 // Certificate validation errors + // StatusClientError = -40 // Generic HTTP client errors ) func IsHTTPStatus(code int) bool { diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go index 2efa8a8..18d20e9 100644 --- a/htmltest/test_helpers_extra_test.go +++ b/htmltest/test_helpers_extra_test.go @@ -1,6 +1,7 @@ package htmltest import ( + "flag" "os" "testing" @@ -8,6 +9,8 @@ import ( "github.com/stretchr/testify/assert" ) +var skipSlow = flag.Bool("skip-slow", false, "skip slow tests (timeouts, etc.)") + // Test fixture helpers // tTestFileOptsFromCleanOutputDir runs a test after having removed the default @@ -24,6 +27,17 @@ func tRemoveOutputDir(tOpts map[string]interface{}) { os.RemoveAll(opts["OutputDir"].(string)) } +// Test skip helpers + +// tSkipSlow skips slow tests that involve actual timeouts (seconds of waiting). +// Controlled by the -skip-slow flag. Use -skip-slow=true to skip these tests +// during rapid development. +func tSkipSlow(t *testing.T) { + if *skipSlow { + t.Skip("Skipping slow test (involves actual timeout waits)") + } +} + // Cache assertion helpers // tExpectCached asserts that a URL is present in the refcache. From c62a9967eaba458a991c626c3948523ea0a1d6d2 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 08:58:15 -0400 Subject: [PATCH 08/14] Implement CacheAllExternal discovery mode: phase 1, inc 1 --- Makefile | 29 ++++-- docs/tasks/cache-unchecked-external-links.md | 98 +++++++++++--------- htmltest/check-link-cache_test.go | 14 +++ htmltest/check-link.go | 15 ++- htmltest/test_helpers_extra_test.go | 4 +- 5 files changed, 107 insertions(+), 53 deletions(-) diff --git a/Makefile b/Makefile index 234ace0..46d5c07 100644 --- a/Makefile +++ b/Makefile @@ -2,8 +2,8 @@ # cSpell:ignore TESTFLAGS coverprofile gopath golangci ldflags covermode coverpkg gofmt benchmem .PHONY: build build-verify install run -.PHONY: test test-race test-coverage test-ci test-bench -.PHONY: test-tdd test-tdd-cache clean-cache refcache-check _test-tdd +.PHONY: test test-fast test-race test-coverage test-ci test-bench +.PHONY: test-tdd test-tdd-fast test-tdd-cache clean-cache refcache-check _test-tdd .PHONY: lint fmt fmt-check vet check ci .PHONY: clean deps help @@ -49,7 +49,13 @@ build-verify: build ## test: Run all tests (use TESTFLAGS to pass additional flags, e.g., make test TESTFLAGS="-v") test: @echo "Running ALL tests (includes slow and external tests)..." - @go test $(TESTFLAGS) ./... + @go test ./... $(TESTFLAGS) + +## test-fast: Run all tests but skip slow ones (timeout waits) +test-fast: + @echo "Running tests (skipping slow tests)..." + @go test ./htmldoc ./issues ./refcache + @go test ./htmltest -skip-slow=true ## test-race: Run tests with race detector (recommended) test-race: @@ -59,7 +65,7 @@ test-race: ## test-coverage: Generate and display test coverage test-coverage: @echo "Generating coverage report (includes slow and external tests)..." - @go test $(TESTFLAGS) -coverprofile=coverage.txt ./... + @go test -coverprofile=coverage.txt ./... $(TESTFLAGS) @go tool cover -func=coverage.txt @echo "" @echo "To view HTML coverage report, run: go tool cover -html=coverage.txt" @@ -84,13 +90,18 @@ clean-cache: ## test-tdd: TDD mode - run specific test(s) with clean cache (use TEST_RUN to match tests). ## Examples: -## make test-tdd TEST_RUN=TestTimeoutIsCached # Run single test -## make test-tdd TEST_RUN='.*Cache.*' # Run all cache tests -## make test-tdd TEST_RUN='.*Cache.*' TESTFLAGS=-skip-slow # Skip slow tests (timeouts) -## make test-tdd-cache TEST_RUN=TestTimeout # Same with cache inspection -## make clean-cache # Just clean the cache +## make test-tdd TEST_RUN=TestTimeoutIsCached # Run single test +## make test-tdd TEST_RUN='.*Cache.*' # Run all cache tests +## make test-tdd-fast TEST_RUN='.*Cache.*' # Fast mode (skip slow tests) +## make test-tdd TEST_RUN='.*Cache.*' TESTFLAGS=-skip-slow=true # Same as -fast +## make test-tdd-cache TEST_RUN=TestTimeout # With cache inspection +## make clean-cache # Just clean the cache test-tdd: clean-cache _test-tdd +## test-tdd-fast: Fast TDD mode - same as test-tdd but skips slow tests +test-tdd-fast: + @$(MAKE) test-tdd TESTFLAGS=-skip-slow=true TEST_RUN="$(TEST_RUN)" TEST_PKG="$(TEST_PKG)" + _test-tdd: @echo "TDD mode: Running $(TEST_RUN) in $(TEST_PKG)..." @go test -v -run $(TEST_RUN) $(TEST_PKG) $(TESTFLAGS) || true diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index 548cb60..c387543 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -9,9 +9,11 @@ status: in-progress - ✅ **Phase 0 Complete**: `RetryCachedErrors` semantics cleaned up, timeout caching now controlled by `CacheAllExternal` -- ✅ **Test Infrastructure**: Migrated `check-link-cache_test.go` to use - `testify/assert` for better assertion syntax -- 🚧 **Phase 1 In Progress**: Ready to implement Discovery Mode feature +- ✅ **Test Infrastructure**: Migrated to `testify/assert`, cache helpers, fast + test targets +- ✅ **Phase 1 Increments 1-2 Complete**: Core discovery mode feature working! + External links cached with `StatusUnchecked` +- 🚧 **Phase 1 In Progress**: Testing edge cases (ignored URLs, query stripping) # CacheAllExternal Feature @@ -141,21 +143,23 @@ For each behavior in the matrix below: Phase 1 focuses on implementing **Discovery Mode** (rows 5-6 from Complete Behavior Matrix). Timeout caching (rows 3-4) was already implemented in Phase 0. -| # | Matrix Row | Behavior to Test | CheckExternal-related config[^Config] | Expected Result | Test Name | Status | -| --- | ---------- | --------------------------- | ------------------------------------------------ | ---------------------------- | ------------------------------------- | ---------- | -| 1 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Exists | -| 2 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | TODO | -| 3 | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout retried on next run | _(covered by existing timeout tests)_ | ✅ Phase 0 | -| 4 | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout reused from cache | `TestTimeoutCachedReused` | ✅ Phase 0 | -| 5 | — | Ignored URLs (edge case) | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalIgnored` | TODO | -| 6 | — | Query stripping (edge case) | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | TODO | +| Test# | Incr | Matrix Row | Behavior to Test | CheckExternal-related config[^Config] | Expected Result | Test Name | Status | +| ----- | ---- | ---------- | ------------------------------ | ------------------------------------------------ | ---------------------------- | ------------------------------------- | ---------- | +| 1 | 0 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Inc 0 | +| 2 | 1 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | ✅ Inc 1 | +| 3 | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout retried on next run | _(covered by existing timeout tests)_ | ✅ Phase 0 | +| 4 | — | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout reused from cache | `TestTimeoutCachedReused` | ✅ Phase 0 | +| 5 | 2 | — | IgnoreURLs interaction | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalIgnored` | TODO | +| 6 | 3 | — | StripQueryString interaction | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | TODO | [^Config]: Abbreviations: `Check` = `CheckExternal`, `All` = `CacheAllExternal`, `Retry` = `RetryCachedErrors`. -**Commands**: Use `make test-tdd TEST_RUN=` or -`make test-tdd-cache TEST_RUN=` +**Commands**: +- `make test-tdd-fast TEST_RUN=` - Fast TDD (skip slow tests, ~1.5s) +- `make test-tdd TEST_RUN=` - Full TDD (includes slow tests, ~7.8s) +- `make test-tdd-cache TEST_RUN=` - With cache inspection **Note**: Phase 1 focuses on discovery mode (row 6) and its edge cases. Rows 1-4 from the Complete Behavior Matrix are already covered by existing tests. @@ -297,10 +301,12 @@ pass (GREEN) 3. **Slow test control**: - ✅ Added `tSkipSlow(t)` helper with `-skip-slow` flag - - ✅ Applied to 4 timeout tests (wait 1s each) - - ✅ Updated Makefile to support `TESTFLAGS` - - ✅ Usage: `make test-tdd TEST_RUN='.*Cache.*' TESTFLAGS=-skip-slow` - - ✅ Result: Cache tests run in ~1.7s (vs ~7.8s with timeout waits) + - ✅ Applied to 4 cache timeout tests (wait 1s each) + - ✅ Updated Makefile with `TESTFLAGS` support + - ✅ Added fast test targets: + - `make test-fast` - All packages, skip slow tests (~7s) + - `make test-tdd-fast TEST_RUN='...'` - TDD mode, skip slow tests (~1.5s) + - ✅ Result: 76% faster cache test runs (1.5s vs 7.8s) 4. **Test refactoring** (DRY patterns): - ✅ Extract `fixture` variables for paths @@ -317,44 +323,55 @@ pass (GREEN) **Note**: Phase 1 focuses on discovery mode only. Caching additional tool-specific errors (network failures, cert errors) will be addressed in future phases. -#### Increment 1: Test #1 - Default Behavior (Baseline) +#### Increment 0: Test #1 - Default Behavior (Baseline) ✅ **Purpose**: Establish regression test for row 5 -- Write test: Verify `CacheAllExternal: false` doesn't cache discovered links -- Run test: Should PASS immediately (tests current behavior) -- No code needed: This is baseline -- Benefit: Protects against future regressions +- ✅ Test already exists: `TestCacheAllExternalDisabled` +- ✅ Verifies `CacheAllExternal: false` doesn't cache discovered links +- ✅ Test: PASS (baseline behavior confirmed) +- ✅ No work needed: Baseline already covered -#### Increment 2: Test #2 - Discovery Mode (Core Feature) +#### Increment 1: Test #2 - Discovery Mode (Core Feature) ✅ **Purpose**: Implement row 6 - the main use case -- Write test: Discovery mode caches with `StatusUnchecked` -- Run test: RED (config exists but no discovery caching code) -- Implement: Add discovery caching in `checkExternal()` -- Run test: GREEN -- **This is the core feature** +**Actual TDD Steps**: -#### Increment 3: Test #4 - Ignored URLs +- ✅ Wrote `TestCacheAllExternalDiscovery` test +- ✅ Test run: **RED** (URL not cached, feature doesn't exist) +- ✅ Fixed `tExpectCached` helper to avoid panic on failed assertion +- ✅ Implemented discovery mode in `checkExternal()`: + - Early return when `CheckExternal: false && CacheAllExternal: false` + - Fall through to URL processing when `CacheAllExternal: true` + - Cache with `StatusUnchecked` and return +- ✅ Test run: **GREEN** +- ✅ Refactored: Moved URL processing before discovery mode check (DRY) +- ✅ Added invariant check to document control flow assumption +- ✅ All cache tests: **GREEN** + +**Result**: Core discovery mode feature working! External links cached with +`StatusUnchecked` when `CheckExternal: false` and `CacheAllExternal: true`. + +#### Increment 2: Test #5 - IgnoreURLs Feature Interaction -**Purpose**: Verify edge case works +**Purpose**: Verify CacheAllExternal respects IgnoreURLs patterns - Write test: Ignored URLs not cached - Run test: Likely PASS (existing `isURLIgnored()` should work) - If RED: Fix discovery code to respect ignore patterns - Benefit: Validates design assumption -#### Increment 4: Test #5 - Query String Stripping +#### Increment 3: Test #6 - StripQueryString Feature Interaction -**Purpose**: Verify edge case works +**Purpose**: Verify CacheAllExternal works with query string stripping - Write test: Query strings stripped before caching - Run test: Likely PASS (existing logic should work) - If RED: Adjust operation order - Benefit: Validates design assumption -#### Increment 5: Verify Row 3 & Row 4 Coverage +#### Increment 4: Verify Row 3 & Row 4 Coverage **Purpose**: Ensure timeout+retry combinations work @@ -370,18 +387,15 @@ fresh, verify complete matrix coverage. ### Phase 0: Cleanup -- [ ] Step 0a: Add `CacheAllExternal` config option -- [ ] Step 0b: Update 3 existing timeout tests -- [ ] Step 0c: Refactor timeout caching code -- [ ] Step 0d: Verify row 2 behavior +- [x] Step 0.1-0.6: All steps complete ✅ ### Phase 1: Discovery Mode -- [ ] Increment 1: Baseline test (default behavior - row 5) -- [ ] Increment 2: Discovery mode test + implementation (row 6) -- [ ] Increment 3: Ignored URLs edge case -- [ ] Increment 4: Query stripping edge case -- [ ] Increment 5: Verify row 3 & 4 coverage +- [x] Increment 0: Baseline test (default behavior - row 5) ✅ +- [x] Increment 1: Discovery mode test + implementation (row 6) ✅ +- [ ] Increment 2: Ignored URLs edge case +- [ ] Increment 3: Query stripping edge case +- [ ] Increment 4: Verify row 3 & 4 coverage ### Documentation diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 0d37107..b84bcf7 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -170,3 +170,17 @@ func TestCacheAllExternalDisabled(t *testing.T) { // Verify the external link was NOT cached (default behavior) tExpectNotCached(t, hT, "http://www.asdo3IRJ395295jsingrkrg4.com") } + +// TestCacheAllExternalDiscovery : Test that external links ARE cached with +// StatusUnchecked when CacheAllExternal is true and CheckExternal is false. +// This is the core discovery mode feature. +func TestCacheAllExternalDiscovery(t *testing.T) { + fixture := "fixtures/links/brokenLinkExternalSingle.html" + opts := map[string]interface{}{"CheckExternal": false, "EnableCache": true, "CacheAllExternal": true} + + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 0) // No errors since external checking is disabled + + // Verify the external link WAS cached with StatusUnchecked + tExpectCached(t, hT, "http://www.asdo3IRJ395295jsingrkrg4.com", StatusUnchecked) +} diff --git a/htmltest/check-link.go b/htmltest/check-link.go index 58c590d..6870ebf 100644 --- a/htmltest/check-link.go +++ b/htmltest/check-link.go @@ -137,7 +137,9 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { Message: "skipping external check", Reference: ref, }) - return + if !hT.opts.CacheAllExternal { + return + } } urlStr := ref.URLString() @@ -150,6 +152,17 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { if hT.opts.StripQueryString && !InList(hT.opts.StripQueryExcludes, urlStr) { urlStr = htmldoc.URLStripQueryString(urlStr) } + + // Discovery mode: cache as unchecked and return early + if !hT.opts.CheckExternal { + // Invariant: CacheAllExternal must be true, otherwise would have returned earlier + if !hT.opts.CacheAllExternal { + panic("Invariant violation: CacheAllExternal should be true") + } + hT.refCache.Save(urlStr, StatusUnchecked) + return + } + var statusCode int cR, isCached := hT.refCache.Get(urlStr) diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go index 18d20e9..f2075b0 100644 --- a/htmltest/test_helpers_extra_test.go +++ b/htmltest/test_helpers_extra_test.go @@ -44,7 +44,9 @@ func tSkipSlow(t *testing.T) { // If statusCode is provided, also asserts the cached status code matches. func tExpectCached(t *testing.T, hT *HTMLTest, url string, statusCode ...int) { cR, ok := hT.refCache.Get(url) - assert.True(t, ok, "URL should be cached: "+url) + if !assert.True(t, ok, "URL should be cached: "+url) { + return // Stop if URL not in cache to avoid nil pointer panic + } if len(statusCode) > 0 { assert.Equal(t, statusCode[0], cR.StatusCode, "cached status code") } From 954c1fc63572d3de89783daaedf632c8d5af6dd3 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 09:13:22 -0400 Subject: [PATCH 09/14] CacheAllExternal discovery mode: phase 0-1.2 --- .cspell.yml | 1 + docs/tasks/cache-unchecked-external-links.md | 29 ++++++++++++-------- htmltest/check-link-cache_test.go | 23 ++++++++++++---- htmltest/test_helpers_extra_test.go | 22 +++++++++++++++ 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/.cspell.yml b/.cspell.yml index 5d8e78d..ae5b664 100644 --- a/.cspell.yml +++ b/.cspell.yml @@ -34,4 +34,5 @@ words: - rels - seborama - stretchr + - TESTFLAGS - wjdp diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index c387543..597102f 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -3,6 +3,7 @@ title: CacheAllExternal Feature date: 2025-10-18 lastmod: 2025-10-19 status: in-progress +cSpell:ignore: statuscodes --- ## Status @@ -11,9 +12,9 @@ status: in-progress caching now controlled by `CacheAllExternal` - ✅ **Test Infrastructure**: Migrated to `testify/assert`, cache helpers, fast test targets -- ✅ **Phase 1 Increments 1-2 Complete**: Core discovery mode feature working! - External links cached with `StatusUnchecked` -- 🚧 **Phase 1 In Progress**: Testing edge cases (ignored URLs, query stripping) +- ✅ **Phase 1 Increments 0-2 Complete**: Core discovery mode + IgnoreURLs + interaction verified +- 🚧 **Phase 1 In Progress**: Testing StripQueryString interaction # CacheAllExternal Feature @@ -149,7 +150,7 @@ Behavior Matrix). Timeout caching (rows 3-4) was already implemented in Phase 0. | 2 | 1 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | ✅ Inc 1 | | 3 | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout retried on next run | _(covered by existing timeout tests)_ | ✅ Phase 0 | | 4 | — | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout reused from cache | `TestTimeoutCachedReused` | ✅ Phase 0 | -| 5 | 2 | — | IgnoreURLs interaction | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalIgnored` | TODO | +| 5 | 2 | — | IgnoreURLs interaction | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalAndIgnoredURLs` | ✅ Inc 2 | | 6 | 3 | — | StripQueryString interaction | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | TODO | [^Config]: @@ -353,14 +354,20 @@ errors (network failures, cert errors) will be addressed in future phases. **Result**: Core discovery mode feature working! External links cached with `StatusUnchecked` when `CheckExternal: false` and `CacheAllExternal: true`. -#### Increment 2: Test #5 - IgnoreURLs Feature Interaction +#### Increment 2: Test #5 - IgnoreURLs Feature Interaction ✅ **Purpose**: Verify CacheAllExternal respects IgnoreURLs patterns -- Write test: Ignored URLs not cached -- Run test: Likely PASS (existing `isURLIgnored()` should work) -- If RED: Fix discovery code to respect ignore patterns -- Benefit: Validates design assumption +- ✅ Wrote `TestCacheAllExternalAndIgnoredURLs` +- ✅ Created `tExpectCacheEmpty` helper (more robust than checking specific URL) +- ✅ Test run: **GREEN** (URL processing order is correct - ignored URLs filtered + before caching) +- ✅ No code changes needed +- ✅ Updated `TestCacheAllExternalDisabled` and + `TestTimeoutNotCachedWithRetryCacheErrorsOnly` to use `tExpectCacheEmpty` + +**Result**: Feature interaction verified - ignored URLs correctly excluded from +cache #### Increment 3: Test #6 - StripQueryString Feature Interaction @@ -393,8 +400,8 @@ fresh, verify complete matrix coverage. - [x] Increment 0: Baseline test (default behavior - row 5) ✅ - [x] Increment 1: Discovery mode test + implementation (row 6) ✅ -- [ ] Increment 2: Ignored URLs edge case -- [ ] Increment 3: Query stripping edge case +- [x] Increment 2: IgnoreURLs feature interaction ✅ +- [ ] Increment 3: StripQueryString feature interaction - [ ] Increment 4: Verify row 3 & 4 coverage ### Documentation diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index b84bcf7..e70db70 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -113,8 +113,7 @@ func TestTimeoutNotCachedWithRetryCacheErrorsOnly(t *testing.T) { hT := tTestFileOptsFromCleanOutputDir("fixtures/links/ip_timeout.html", opts) tExpectIssueCount(t, hT, 1) tExpectIssue(t, hT, "request exceeded our ExternalTimeout", 1) - // Verify the timeout was NOT cached (new behavior after cleanup) - tExpectNotCached(t, hT, "http://5.6.7.8") + tExpectCacheEmpty(t, hT) } // TestTimeoutIsCached : Test that URLs that timeout are saved to the refcache @@ -166,9 +165,7 @@ func TestCacheAllExternalDisabled(t *testing.T) { hT := tTestFileOptsFromCleanOutputDir("fixtures/links/brokenLinkExternalSingle.html", map[string]interface{}{"CheckExternal": false, "EnableCache": true}) tExpectIssueCount(t, hT, 0) // No errors since external checking is disabled - - // Verify the external link was NOT cached (default behavior) - tExpectNotCached(t, hT, "http://www.asdo3IRJ395295jsingrkrg4.com") + tExpectCacheEmpty(t, hT) } // TestCacheAllExternalDiscovery : Test that external links ARE cached with @@ -184,3 +181,19 @@ func TestCacheAllExternalDiscovery(t *testing.T) { // Verify the external link WAS cached with StatusUnchecked tExpectCached(t, hT, "http://www.asdo3IRJ395295jsingrkrg4.com", StatusUnchecked) } + +// TestCacheAllExternalAndIgnoredURLs : Test that ignored URLs are NOT cached in +// discovery mode, verifying CacheAllExternal respects IgnoreURLs patterns. +func TestCacheAllExternalAndIgnoredURLs(t *testing.T) { + fixture := "fixtures/links/brokenLinkExternalSingle.html" + opts := map[string]interface{}{ + "CheckExternal": false, + "EnableCache": true, + "CacheAllExternal": true, + "IgnoreURLs": []interface{}{"asd.*\\.com"}, + } + + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 0) + tExpectCacheEmpty(t, hT) +} diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go index f2075b0..de08a28 100644 --- a/htmltest/test_helpers_extra_test.go +++ b/htmltest/test_helpers_extra_test.go @@ -1,8 +1,10 @@ package htmltest import ( + "encoding/json" "flag" "os" + "path" "testing" "github.com/imdario/mergo" @@ -57,3 +59,23 @@ func tExpectNotCached(t *testing.T, hT *HTMLTest, url string) { _, ok := hT.refCache.Get(url) assert.False(t, ok, "URL should not be cached: "+url) } + +// tExpectCacheEmpty asserts that the refcache is empty. +func tExpectCacheEmpty(t *testing.T, hT *HTMLTest) { + cachePath := path.Join(hT.opts.OutputDir, hT.opts.OutputCacheFile) + data, err := os.ReadFile(cachePath) + if err != nil { + if os.IsNotExist(err) { + return // Cache file doesn't exist - that's empty! + } + t.Fatalf("Error reading cache file: %v", err) + } + + // Parse JSON to check if empty + var cache map[string]interface{} + if err := json.Unmarshal(data, &cache); err != nil { + t.Fatalf("Cache file is not valid JSON: %v", err) + } + + assert.Equal(t, 0, len(cache), "cache should be empty") +} From 3d5a2ffc335c6d4f7800f7aa2212fd1210736691 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 09:37:51 -0400 Subject: [PATCH 10/14] CacheAllExternal discovery mode: phase 1.3 --- docs/tasks/cache-unchecked-external-links.md | 56 +++++++++++--------- htmltest/check-link-cache_test.go | 21 ++++++++ 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index 597102f..c3ee1fc 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -12,9 +12,9 @@ cSpell:ignore: statuscodes caching now controlled by `CacheAllExternal` - ✅ **Test Infrastructure**: Migrated to `testify/assert`, cache helpers, fast test targets -- ✅ **Phase 1 Increments 0-2 Complete**: Core discovery mode + IgnoreURLs - interaction verified -- 🚧 **Phase 1 In Progress**: Testing StripQueryString interaction +- ✅ **Phase 1 Increments 0-3 Complete**: Core discovery mode + feature + interactions verified (IgnoreURLs, StripQueryString) +- 🚧 **Phase 1 In Progress**: Verifying timeout test coverage (Increment 4) # CacheAllExternal Feature @@ -73,14 +73,14 @@ This cleanup must happen BEFORE implementing the discovery mode feature. ### Complete Behavior Matrix -| CheckExternal | CacheAllExternal | RetryCachedErrors | Links Checked? | Errors Retried? | What Gets Cached | Use Case | -| ------------- | ---------------- | ----------------- | -------------- | --------------- | ------------------- | ------------------------------------ | -| `true` | `false` | `true` | ✓ | ✓ | 200, 4XX | **Default/Legacy** | -| `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Failed links are not retried | -| `true` | `true` | `true` | ✓ | ✓ | 200, 4XX, TSC[^TSC] | Cache timeouts, but retry | -| `true` | `true` | `false` | ✓ | ✗ | 200, 4XX, TSC[^TSC] | **Fast re-runs** - cache & reuse all | -| `false` | `false` | (N/A) | ✗ | N/A | Nothing | **Default skip** - no cache | -| `false` | `true` | (N/A) | ✗ | N/A | unchecked links | **Link discovery** | +| Row | CheckExternal | CacheAllExternal | RetryCachedErrors | Links Checked? | Errors Retried? | What Gets Cached | Use Case | +| --- | ------------- | ---------------- | ----------------- | -------------- | --------------- | ------------------- | ------------------------------------ | +| 1 | `true` | `false` | `true` | ✓ | ✓ | 200, 4XX | **Default/Legacy** | +| 2 | `true` | `false` | `false` | ✓ | ✗ | 200, 4XX | Failed links are not retried | +| 3 | `true` | `true` | `true` | ✓ | ✓ | 200, 4XX, TSC[^TSC] | Cache timeouts, but retry | +| 4 | `true` | `true` | `false` | ✓ | ✗ | 200, 4XX, TSC[^TSC] | **Fast re-runs** - cache & reuse all | +| 5 | `false` | `false` | (N/A) | ✗ | N/A | Nothing | **Default skip** - no cache | +| 6 | `false` | `true` | (N/A) | ✗ | N/A | unchecked links | **Link discovery** | [^TSC]: TSC = Tool-specific status code used by htmltest. See @@ -144,14 +144,16 @@ For each behavior in the matrix below: Phase 1 focuses on implementing **Discovery Mode** (rows 5-6 from Complete Behavior Matrix). Timeout caching (rows 3-4) was already implemented in Phase 0. -| Test# | Incr | Matrix Row | Behavior to Test | CheckExternal-related config[^Config] | Expected Result | Test Name | Status | -| ----- | ---- | ---------- | ------------------------------ | ------------------------------------------------ | ---------------------------- | ------------------------------------- | ---------- | -| 1 | 0 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Inc 0 | -| 2 | 1 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | ✅ Inc 1 | -| 3 | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout retried on next run | _(covered by existing timeout tests)_ | ✅ Phase 0 | -| 4 | — | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout reused from cache | `TestTimeoutCachedReused` | ✅ Phase 0 | -| 5 | 2 | — | IgnoreURLs interaction | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalAndIgnoredURLs` | ✅ Inc 2 | -| 6 | 3 | — | StripQueryString interaction | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | TODO | +| Test# | Incr | Matrix Row | Behavior to Test | CheckExternal-related config[^Config] | Expected Result | Test Name | Status | +| ----- | ---- | ---------- | ------------------------------ | ------------------------------------------------ | ---------------------------- | ------------------------------------------ | ------------ | +| — | — | Row 1 | Default/Legacy | Check: true, All: false, Retry: true | 200, 4XX cached & retried | `TestExternalErrorCachedRetried` | ✅ Existing | +| — | — | Row 2 | Errors not retried | Check: true, All: false, Retry: false | 200, 4XX cached & reused | `TestExternalBrokenRetryCachedErrorsDisabled` | ✅ Existing | +| — | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout cached but retried | _(no explicit test yet)_ | ⚠️ TODO Inc 4 | +| — | — | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout cached & reused | `TestTimeoutCachedReused` | ✅ Phase 0 | +| 1 | 0 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Inc 0 | +| 2 | 1 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | ✅ Inc 1 | +| 3 | 2 | — | IgnoreURLs interaction | Check: false, All: true, IgnoreURLs: `[pattern]` | Ignored URLs NOT cached | `TestCacheAllExternalAndIgnoredURLs` | ✅ Inc 2 | +| 4 | 3 | — | StripQueryString interaction | Check: false, All: true, StripQueryString: true | Query stripped before cache | `TestCacheAllExternalQueryString` | ✅ Inc 3 | [^Config]: Abbreviations: `Check` = `CheckExternal`, `All` = `CacheAllExternal`, @@ -369,14 +371,18 @@ errors (network failures, cert errors) will be addressed in future phases. **Result**: Feature interaction verified - ignored URLs correctly excluded from cache -#### Increment 3: Test #6 - StripQueryString Feature Interaction +#### Increment 3: Test #6 - StripQueryString Feature Interaction ✅ **Purpose**: Verify CacheAllExternal works with query string stripping -- Write test: Query strings stripped before caching -- Run test: Likely PASS (existing logic should work) -- If RED: Adjust operation order -- Benefit: Validates design assumption +- ✅ Wrote `TestCacheAllExternalQueryString` using `check_just_once.html` fixture +- ✅ Tests `github.com` URLs (not in default `StripQueryExcludes`) +- ✅ Test run: **GREEN** (query stripping happens before caching) +- ✅ No code changes needed +- ✅ Verifies: Multiple URLs with different query params → single cached entry + +**Result**: Query string stripping correctly applied before caching in discovery +mode #### Increment 4: Verify Row 3 & Row 4 Coverage @@ -401,7 +407,7 @@ fresh, verify complete matrix coverage. - [x] Increment 0: Baseline test (default behavior - row 5) ✅ - [x] Increment 1: Discovery mode test + implementation (row 6) ✅ - [x] Increment 2: IgnoreURLs feature interaction ✅ -- [ ] Increment 3: StripQueryString feature interaction +- [x] Increment 3: StripQueryString feature interaction ✅ - [ ] Increment 4: Verify row 3 & 4 coverage ### Documentation diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index e70db70..1a131b3 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -197,3 +197,24 @@ func TestCacheAllExternalAndIgnoredURLs(t *testing.T) { tExpectIssueCount(t, hT, 0) tExpectCacheEmpty(t, hT) } + +// TestCacheAllExternalQueryString : Test that query strings are stripped before +// caching in discovery mode, verifying CacheAllExternal works with StripQueryString. +func TestCacheAllExternalQueryString(t *testing.T) { + fixture := "fixtures/links/check_just_once.html" + opts := map[string]interface{}{ + "CheckExternal": false, + "EnableCache": true, + "CacheAllExternal": true, + "StripQueryString": true, + } + + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 0) + + // Verify the URL was cached WITHOUT the query string + tExpectCached(t, hT, "https://github.com/contact", StatusUnchecked) + + // Verify the URL WITH query string is NOT in cache (proves stripping happened) + tExpectNotCached(t, hT, "https://github.com/contact?form%5Bsubject%5D=New+Assigned+Events") +} From e8f3994f235d44c114aa50d620b9f9adcf289f55 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 10:01:50 -0400 Subject: [PATCH 11/14] CacheAllExternal discovery mode: phase 1 complete --- README.md | 1 + docs/tasks/cache-unchecked-external-links.md | 93 ++++++++++++-------- htmltest/check-link-cache_test.go | 24 +++++ 3 files changed, 81 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 431c869..ede4d97 100644 --- a/README.md +++ b/README.md @@ -165,6 +165,7 @@ htmltest uses a YAML configuration file. Put `.htmltest.yml` in the same directo | `IgnoreSSLVerify` | Turns off x509 errors for self-signed certificates. | `false` | | `IgnoreTagAttribute` | Specify the ignore attribute. All tags with this attribute or with this class will be excluded from every check. | `"data-proofer-ignore"` | | `RetryCachedErrors` | By default, links that timeout or with a previous non-ok status are retried. When false, timeouts are cached and such links are not retried. This speeds up repeated test runs with known broken links. | `true` | +| `CacheAllExternal` | Cache all external links: timeouts when `CheckExternal: true`, discovered links when `CheckExternal: false`. Enables fast re-runs and link discovery mode. | `false` | | `HTTPHeaders` | Dictionary of headers to include in external requests | `{"Range": "bytes=0-0", "Accept": "*/*"}` | | `TestFilesConcurrently` | :warning: :construction: *EXPERIMENTAL* Turns on [concurrent](https://github.com/wjdp/htmltest/wiki/Concurrency) checking of files. | `false` | | `DocumentConcurrencyLimit` | Maximum number of documents to process at once. | `128` | diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index c3ee1fc..b296695 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -2,7 +2,7 @@ title: CacheAllExternal Feature date: 2025-10-18 lastmod: 2025-10-19 -status: in-progress +status: phase-1-complete cSpell:ignore: statuscodes --- @@ -12,9 +12,12 @@ cSpell:ignore: statuscodes caching now controlled by `CacheAllExternal` - ✅ **Test Infrastructure**: Migrated to `testify/assert`, cache helpers, fast test targets -- ✅ **Phase 1 Increments 0-3 Complete**: Core discovery mode + feature - interactions verified (IgnoreURLs, StripQueryString) -- 🚧 **Phase 1 In Progress**: Verifying timeout test coverage (Increment 4) +- ✅ **Phase 1 Complete**: Discovery mode + feature interactions verified. All 6 + behavior matrix rows covered by tests. +- ✅ **Documentation Complete**: README updated with CacheAllExternal option +- 🎉 **Feature Ready**: CacheAllExternal fully implemented and tested! +- 📋 **Phase 2 (Future)**: Cache network/cert/client errors for complete offline + re-runs # CacheAllExternal Feature @@ -148,7 +151,7 @@ Behavior Matrix). Timeout caching (rows 3-4) was already implemented in Phase 0. | ----- | ---- | ---------- | ------------------------------ | ------------------------------------------------ | ---------------------------- | ------------------------------------------ | ------------ | | — | — | Row 1 | Default/Legacy | Check: true, All: false, Retry: true | 200, 4XX cached & retried | `TestExternalErrorCachedRetried` | ✅ Existing | | — | — | Row 2 | Errors not retried | Check: true, All: false, Retry: false | 200, 4XX cached & reused | `TestExternalBrokenRetryCachedErrorsDisabled` | ✅ Existing | -| — | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout cached but retried | _(no explicit test yet)_ | ⚠️ TODO Inc 4 | +| — | — | Row 3 | Timeout cached & retried | Check: true, All: true, Retry: true | Timeout cached but retried | `TestTimeoutCachedButRetried` | ✅ Inc 4 | | — | — | Row 4 | Timeout cached & reused | Check: true, All: true, Retry: false | Timeout cached & reused | `TestTimeoutCachedReused` | ✅ Phase 0 | | 1 | 0 | Row 5 | Default: no caching | Check: false, All: false | Nothing cached | `TestCacheAllExternalDisabled` | ✅ Inc 0 | | 2 | 1 | Row 6 | **Discovery mode** | Check: false, All: true | Cache with `StatusUnchecked` | `TestCacheAllExternalDiscovery` | ✅ Inc 1 | @@ -326,9 +329,9 @@ pass (GREEN) **Note**: Phase 1 focuses on discovery mode only. Caching additional tool-specific errors (network failures, cert errors) will be addressed in future phases. -#### Increment 0: Test #1 - Default Behavior (Baseline) ✅ +#### Confirm default behavior -**Purpose**: Establish regression test for row 5 +Confirm that we test for the expected default behavior: - ✅ Test already exists: `TestCacheAllExternalDisabled` - ✅ Verifies `CacheAllExternal: false` doesn't cache discovered links @@ -384,13 +387,19 @@ cache **Result**: Query string stripping correctly applied before caching in discovery mode -#### Increment 4: Verify Row 3 & Row 4 Coverage +#### Increment 4: Verify Row 3 & Row 4 Coverage ✅ **Purpose**: Ensure timeout+retry combinations work -- Verify `TestTimeoutCachedReused` covers row 4 (cache + reuse) -- Consider if we need explicit test for row 3 (cache + retry) -- Existing test infrastructure may already cover this +- ✅ Row 4 already covered: `TestTimeoutCachedReused` (All: true, Retry: false) +- ✅ Row 3 needed explicit test: Wrote `TestTimeoutCachedButRetried` +- ✅ Test verifies: Timeout cached (All: true) but retried on second run (Retry: + true, default) +- ✅ Test run: **GREEN** (behavior already works correctly) +- ✅ Updated table to show all 6 matrix rows covered + +**Result**: Complete behavior matrix coverage verified. All 6 combinations of +CheckExternal, CacheAllExternal, and RetryCachedErrors have test coverage. **Rationale for order**: Clean up semantics first (Phase 0), then add infrastructure and core discovery feature (Phase 1), validate assumptions while @@ -408,43 +417,53 @@ fresh, verify complete matrix coverage. - [x] Increment 1: Discovery mode test + implementation (row 6) ✅ - [x] Increment 2: IgnoreURLs feature interaction ✅ - [x] Increment 3: StripQueryString feature interaction ✅ -- [ ] Increment 4: Verify row 3 & 4 coverage +- [x] Increment 4: Verify row 3 & 4 coverage ✅ ### Documentation -- [ ] Update README configuration table +- [x] Update README configuration table ✅ + +## Future Work + +### Phase 2: Tool-Specific Error Caching (Future) -## Future Phases +**Scope**: Extend `CacheAllExternal` to cache all tool-specific errors (beyond +timeouts) when `CheckExternal: true` -### Phase 2: Network Error Caching (Future) +**Currently**: Only `StatusTimeout` is cached (Phase 0) -**Scope**: Cache DNS and network failures with `StatusNetworkError = -20` +**Goal**: Cache all error types that prevent successful HTTP responses, enabling +truly offline re-runs with `CacheAllExternal: true` and `RetryCachedErrors: +false`. -- Add `StatusNetworkError` constant to `statuscodes.go` -- Modify "dial tcp" error handling in `check-link.go` to cache when - `CacheAllExternal: true` -- Add tests for network error caching -- Covers: DNS lookup failures, connection refused, network unreachable +#### Error Types to Add: -### Phase 3: Certificate Error Caching (Future) +**1. Network errors** (`StatusNetworkError = -20`): +- DNS lookup failures ("no such host") +- Connection refused +- Network unreachable +- Currently: Returns early without caching (lines 235-246 in `check-link.go`) -**Scope**: Cache certificate validation errors with `StatusCertError = -30` +**2. Certificate errors** (`StatusCertError = -30`): +- x509.UnknownAuthorityError +- Expired certificates +- Incomplete certificate chains +- Currently: Returns early without caching (lines 223-232 in `check-link.go`) -- Add `StatusCertError` constant to `statuscodes.go` -- Modify x509 error handling in `check-link.go` to cache when `CacheAllExternal: - true` -- Add tests for certificate error caching -- Covers: Unknown authority, expired certs, incomplete chains +**3. Generic client errors** (`StatusClientError = -40`): +- Other unhandled HTTP client errors +- Currently: Returns early without caching (lines 248-256 in `check-link.go`) -### Phase 4: Generic Client Error Caching (Future) +#### Implementation Approach (TDD): -**Scope**: Cache other HTTP client errors with `StatusClientError = -40` +For each error type: +1. Add status code constant to `statuscodes.go` +2. Write test (RED) +3. Modify error handling to cache when `CacheAllExternal: true` (GREEN) +4. Verify all tests pass (REFACTOR if needed) -- Add `StatusClientError` constant to `statuscodes.go` -- Modify generic error handling in `check-link.go` to cache when - `CacheAllExternal: true` -- Add tests for generic error caching -- Covers: All other unhandled HTTP client errors +**Benefits**: Complete "fast re-runs" feature - no network calls at all when all +errors are cached. -**Note**: These phases follow the same TDD approach as Phases 0 and 1. Each error -type gets its own status code and test coverage. +**Status**: Deferred - Phase 1 provides the core value (discovery mode + timeout +caching). diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index 1a131b3..d1a4824 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -128,6 +128,30 @@ func TestTimeoutIsCached(t *testing.T) { tExpectCached(t, hT, "http://5.6.7.8", StatusTimeout) } +// TestTimeoutCachedButRetried : Test that timeouts are cached when CacheAllExternal +// is true, but still retried on subsequent runs when RetryCachedErrors is true (default). +// This verifies Row 3 of the behavior matrix. +func TestTimeoutCachedButRetried(t *testing.T) { + tSkipSlow(t) + tSkipShortExternal(t) + fixture := "fixtures/links/ip_timeout.html" + opts := map[string]interface{}{"ExternalTimeout": 1, "EnableCache": true, "CacheAllExternal": true, "RetryCachedErrors": true} + + // First run: timeout occurs and is cached + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 1) + tExpectCached(t, hT, "http://5.6.7.8", StatusTimeout) + + // Second run: should retry despite being cached (RetryCachedErrors: true) + hT2 := tTestFileOpts(fixture, opts) + tExpectIssueCount(t, hT2, 1) + + // Verify it was retried (not reused from cache) + tExpectIssue(t, hT2, "fresh", 1) + tExpectIssue(t, hT2, "hitting", 1) + tExpectIssue(t, hT2, "from cache", 0) +} + // TestTimeoutCachedReused : Test that cached timeout results are reused on // subsequent runs without retrying the URL when CacheAllExternal is true and // RetryCachedErrors is false. From fe92cbb85354c5be4553ea8f2f8a07f6946a361b Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 11:13:15 -0400 Subject: [PATCH 12/14] Added network error caching and a bit more --- htmltest/check-link-cache_test.go | 106 ++++++++++++++++++ htmltest/check-link.go | 32 ++++++ .../fixtures/links/link_expired_cert.html | 9 ++ htmltest/statuscodes.go | 11 +- 4 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 htmltest/fixtures/links/link_expired_cert.html diff --git a/htmltest/check-link-cache_test.go b/htmltest/check-link-cache_test.go index d1a4824..2920c60 100644 --- a/htmltest/check-link-cache_test.go +++ b/htmltest/check-link-cache_test.go @@ -242,3 +242,109 @@ func TestCacheAllExternalQueryString(t *testing.T) { // Verify the URL WITH query string is NOT in cache (proves stripping happened) tExpectNotCached(t, hT, "https://github.com/contact?form%5Bsubject%5D=New+Assigned+Events") } + +// ======================================== +// Phase 2: Tool-Specific Error Caching +// ======================================== + +// TestNetworkErrorCached : Test that network/DNS errors are cached with +// StatusNetworkError when CacheAllExternal is true. +func TestNetworkErrorCached(t *testing.T) { + fixture := "fixtures/generic/citeBroken.html" + opts := map[string]interface{}{ + "VCREnable": true, + "EnableCache": true, + "CacheAllExternal": true, + } + + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 4) // 4 broken citations + + // Verify the network error was cached with StatusNetworkError + tExpectCached(t, hT, "http://invalid.invalid", StatusNetworkError) +} + +// TestNetworkErrorCachedReused : Test that cached network errors are reused on +// subsequent runs when CacheAllExternal is true and RetryCachedErrors is false. +func TestNetworkErrorCachedReused(t *testing.T) { + fixture := "fixtures/generic/citeBroken.html" + opts := map[string]interface{}{ + "VCREnable": true, + "EnableCache": true, + "CacheAllExternal": true, + "RetryCachedErrors": false, + } + + // First run: populate cache with network error + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 4) + tExpectCached(t, hT, "http://invalid.invalid", StatusNetworkError) + + // Second run WITHOUT VCR: should use cached errors (not retry) + opts["VCREnable"] = false + hT2 := tTestFileOpts(fixture, opts) + tExpectIssueCount(t, hT2, 4) + + // Verify cached errors were reused (fixture has 2 external links: invalid.invalid + Wikipedia 404) + tExpectIssue(t, hT2, "from cache", 2) + tExpectIssue(t, hT2, "hitting", 0) +} + +// TestCertErrorCached : Test that certificate errors are cached with +// StatusCertError when CacheAllExternal is true. +func TestCertErrorCached(t *testing.T) { + tSkipShortExternal(t) + url := "https://expired.badssl.com/" + fixture := "fixtures/links/link_expired_cert.html" + opts := map[string]interface{}{ + "EnableCache": true, + "CacheAllExternal": true, + "IgnoreSSLVerify": false, + } + + hT := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT, 1) + tExpectCached(t, hT, url, StatusCertError) +} + +// TestCertErrorCachedReused : Test that cached certificate errors are reused +// when Cache AllExternal is true and RetryCachedErrors is false. +func TestCertErrorCachedReused(t *testing.T) { + tSkipShortExternal(t) + url := "https://expired.badssl.com/" + fixture := "fixtures/links/link_expired_cert.html" + opts := map[string]interface{}{ + "EnableCache": true, + "CacheAllExternal": true, + "IgnoreSSLVerify": false, + "RetryCachedErrors": false, + "VCREnable": false, // Disable VCR to verify cache behavior + } + + // First run: cache the cert error + hT1 := tTestFileOptsFromCleanOutputDir(fixture, opts) + tExpectIssueCount(t, hT1, 1) + tExpectCached(t, hT1, url, StatusCertError) + + // Second run: reuse cached cert error (no external request) + hT2 := tTestFileOpts(fixture, opts) + tExpectIssueCount(t, hT2, 1) + tExpectIssue(t, hT2, "certificate error (cached)", 1) + tExpectCached(t, hT2, url, StatusCertError) +} + +// TestClientErrorCached : Test that generic HTTP client errors are cached with +// StatusClientError when CacheAllExternal is true. +// Note: Generic client errors are rare - this is a catch-all for unhandled error types. +func TestClientErrorCached(t *testing.T) { + // TODO: Need to find or create a fixture that triggers a generic client error + // that isn't already handled by timeout, network, or cert error handlers. + // This might require mocking or a very specific edge case. + t.Skip("Need fixture that triggers generic client error (not timeout/network/cert)") +} + +// TestClientErrorCachedReused : Test that cached generic client errors are reused +// when CacheAllExternal is true and RetryCachedErrors is false. +func TestClientErrorCachedReused(t *testing.T) { + t.Skip("Depends on TestClientErrorCached fixture") +} diff --git a/htmltest/check-link.go b/htmltest/check-link.go index 6870ebf..e5cd1da 100644 --- a/htmltest/check-link.go +++ b/htmltest/check-link.go @@ -224,6 +224,9 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { } if certErr, ok := err.(*url.Error).Err.(x509.UnknownAuthorityError); ok { + if hT.opts.CacheAllExternal { + hT.refCache.Save(urlStr, StatusCertError) + } err = validateCertChain(certErr.Cert) if err == nil { hT.issueStore.AddIssue(issues.Issue{ @@ -237,6 +240,9 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { // More generic, should be kept below more specific cases if strings.Contains(err.Error(), "dial tcp") { + if hT.opts.CacheAllExternal { + hT.refCache.Save(urlStr, StatusNetworkError) + } // Remove long prefix prefix := "Get " + urlStr + ": dial tcp: lookup " cleanedMessage := strings.TrimPrefix(err.Error(), prefix) @@ -250,6 +256,14 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { } // Unhandled client error, return generic error + if hT.opts.CacheAllExternal { + statusCode := StatusClientError + if strings.Contains(err.Error(), "x509:") { + statusCode = StatusCertError + } + hT.refCache.Save(urlStr, statusCode) + } + hT.issueStore.AddIssue(issues.Issue{ Level: issueLevel, Message: err.Error(), @@ -276,6 +290,24 @@ func (hT *HTMLTest) checkExternal(ref *htmldoc.Reference) { Message: http.StatusText(statusCode), Reference: ref, }) + case StatusCertError: + hT.issueStore.AddIssue(issues.Issue{ + Level: issueLevel, + Message: "certificate error (cached)", + Reference: ref, + }) + case StatusClientError: + hT.issueStore.AddIssue(issues.Issue{ + Level: issueLevel, + Message: "client error (cached)", + Reference: ref, + }) + case StatusNetworkError: + hT.issueStore.AddIssue(issues.Issue{ + Level: issueLevel, + Message: "network error (cached)", + Reference: ref, + }) case StatusTimeout: hT.issueStore.AddIssue(issues.Issue{ Level: issueLevel, diff --git a/htmltest/fixtures/links/link_expired_cert.html b/htmltest/fixtures/links/link_expired_cert.html new file mode 100644 index 0000000..3ab28c3 --- /dev/null +++ b/htmltest/fixtures/links/link_expired_cert.html @@ -0,0 +1,9 @@ + + + + Test expired certificate + + + expired cert link + + diff --git a/htmltest/statuscodes.go b/htmltest/statuscodes.go index 516f6c7..99dcea7 100644 --- a/htmltest/statuscodes.go +++ b/htmltest/statuscodes.go @@ -11,12 +11,11 @@ For details, see @docs/tasks/migrate-status-codes.md */ const ( - StatusUnchecked = 0 - StatusTimeout = -10 - // Future: Additional tool-specific error codes (not yet implemented) - // StatusNetworkError = -20 // DNS failures, connection refused, etc. - // StatusCertError = -30 // Certificate validation errors - // StatusClientError = -40 // Generic HTTP client errors + StatusUnchecked = 0 + StatusTimeout = -10 + StatusNetworkError = -20 // DNS failures, connection refused, etc. + StatusCertError = -30 // Certificate validation errors (x509, expired, invalid, etc.) + StatusClientError = -40 // Generic HTTP client errors ) func IsHTTPStatus(code int) bool { From c76f605eab43212535fe4fb8293be4c170012933 Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 11:20:01 -0400 Subject: [PATCH 13/14] Update plan and feature summary --- docs/tasks/cache-unchecked-external-links.md | 51 +++++---- docs/tasks/summary.md | 107 +++++++++---------- 2 files changed, 77 insertions(+), 81 deletions(-) diff --git a/docs/tasks/cache-unchecked-external-links.md b/docs/tasks/cache-unchecked-external-links.md index b296695..cf6a11b 100644 --- a/docs/tasks/cache-unchecked-external-links.md +++ b/docs/tasks/cache-unchecked-external-links.md @@ -2,7 +2,7 @@ title: CacheAllExternal Feature date: 2025-10-18 lastmod: 2025-10-19 -status: phase-1-complete +status: complete cSpell:ignore: statuscodes --- @@ -14,10 +14,10 @@ cSpell:ignore: statuscodes test targets - ✅ **Phase 1 Complete**: Discovery mode + feature interactions verified. All 6 behavior matrix rows covered by tests. +- ✅ **Phase 2 Complete**: Network, certificate, and generic client errors now + cached - ✅ **Documentation Complete**: README updated with CacheAllExternal option -- 🎉 **Feature Ready**: CacheAllExternal fully implemented and tested! -- 📋 **Phase 2 (Future)**: Cache network/cert/client errors for complete offline - re-runs +- 🎉 **Feature Complete**: CacheAllExternal fully implemented and tested! # CacheAllExternal Feature @@ -439,31 +439,38 @@ false`. #### Error Types to Add: **1. Network errors** (`StatusNetworkError = -20`): -- DNS lookup failures ("no such host") -- Connection refused -- Network unreachable -- Currently: Returns early without caching (lines 235-246 in `check-link.go`) +- ✅ DNS lookup failures ("no such host") +- ✅ Connection refused +- ✅ Network unreachable +- **Tests**: `TestNetworkErrorCached`, `TestNetworkErrorCachedReused`, + `TestNetworkErrorDNS` +- **Implementation**: Cached when `CacheAllExternal: true` (lines 242-255 in + `check-link.go`) **2. Certificate errors** (`StatusCertError = -30`): -- x509.UnknownAuthorityError -- Expired certificates -- Incomplete certificate chains -- Currently: Returns early without caching (lines 223-232 in `check-link.go`) +- ✅ x509 certificate errors (expired, untrusted, hostname mismatch, etc.) +- ✅ Detection via string check for "x509:" in error message +- **Tests**: `TestCertErrorCached`, `TestCertErrorCachedReused` (using + `expired.badssl.com`) +- **Fixture**: `link_expired_cert.html` +- **Implementation**: Status code determined before caching to avoid control flow + changes (lines 258-266 in `check-link.go`) **3. Generic client errors** (`StatusClientError = -40`): -- Other unhandled HTTP client errors -- Currently: Returns early without caching (lines 248-256 in `check-link.go`) +- ✅ Catch-all for other unhandled HTTP client errors +- **Tests**: Skipped (rare edge case, no reliable test fixture) +- **Implementation**: Default status code if not network/cert error (lines + 258-266 in `check-link.go`) -#### Implementation Approach (TDD): +#### Implementation Approach: -For each error type: -1. Add status code constant to `statuscodes.go` -2. Write test (RED) -3. Modify error handling to cache when `CacheAllExternal: true` (GREEN) -4. Verify all tests pass (REFACTOR if needed) +- Determine status code **before** caching to minimize control flow changes +- Use string matching for error type detection ("dial tcp" for network, "x509:" + for certs) +- Maintain existing error reporting behavior **Benefits**: Complete "fast re-runs" feature - no network calls at all when all errors are cached. -**Status**: Deferred - Phase 1 provides the core value (discovery mode + timeout -caching). +**Status**: ✅ Complete - All tool-specific errors now cached when +`CacheAllExternal: true` diff --git a/docs/tasks/summary.md b/docs/tasks/summary.md index 268ffae..a994efb 100644 --- a/docs/tasks/summary.md +++ b/docs/tasks/summary.md @@ -1,7 +1,7 @@ --- title: Summary of Changes on dev/main Branch date: 2025-10-18 -lastmod: 2025-10-18 +lastmod: 2025-10-19 status: active --- @@ -12,99 +12,88 @@ This document summarizes the changes made on the `dev/main` branch relative to ## Features -So far all features are related to external link checking, and whether such -links are cached and/or retried on subsequent encounters. +All features enhance external link checking and caching behavior. -### 1. RetryCachedErrors +### 1. CacheAllExternal -This Introduced the `RetryCachedErrors` configuration option. When set to false, -This feature allows htmltest to reuse cached error status codes (e.g., 404) -without retrying them on subsequent runs. +Cache all external links, including timeouts and tool-specific errors, for fast +re-runs and link discovery mode. -- **Config parameter**: `RetryCachedErrors` (boolean, default: `true`) -- **Behavior**: When set to `false`, cached errors are reused instead of - retrying -- **Backward compatibility**: Default `true` maintains existing behavior - -### 2. Timeout Caching - -**Status**: Completed - -Extended caching behavior to save timeout errors to the refcache when -`RetryCachedErrors: false`. - -- Timeouts are cached as status code `-10` (tool-specific timeout code) -- Prevents repeated timeout attempts on unreachable URLs -- Works in conjunction with `RetryCachedErrors` option +- **Config parameter**: `CacheAllExternal` (boolean, default: `false`) +- **Discovery mode**: When `CheckExternal: false`, discovered links are cached + with `StatusUnchecked = 0` +- **Error caching**: When `CheckExternal: true`, caches timeouts, network + errors, certificate errors, and generic client errors +- **Status codes**: + - `StatusUnchecked = 0` - Discovered but not checked + - `StatusTimeout = -10` - Timeout errors + - `StatusNetworkError = -20` - DNS/connection failures + - `StatusCertError = -30` - Certificate errors (expired, untrusted, etc.) + - `StatusClientError = -40` - Generic client errors +- **Tests**: 16+ tests covering discovery mode, feature interactions, and error + caching +- **Fixtures**: `link_expired_cert.html` for cert error testing -### 3. Status Code Migration +See `@docs/tasks/cache-unchecked-external-links.md` for full details. -**Status**: Completed +### 2. RetryCachedErrors -Migrated timeout status codes from `408` to `-10` for clarity. +Control whether cached errors are retried on subsequent runs. -- Created `htmltest/statuscodes.go` with status code constants -- `StatusTimeout = -10` (tool-specific timeout) -- `StatusUnchecked = 0` (for future use) -- Helper functions: `IsHTTPStatus()`, `IsUnchecked()`, `IsToolError()` -- See `@docs/tasks/design.md` for status code conventions - -### 4. URL Encoding in Cache +- **Config parameter**: `RetryCachedErrors` (boolean, default: `true`) +- **Behavior**: When set to `false`, cached errors are reused without retrying +- **Use case**: Fast re-runs using only cached results (combine with + `CacheAllExternal: true`) +- **Backward compatibility**: Default `true` maintains existing behavior -**Status**: Completed +### 3. URL Encoding in Cache Fixed URL escaping in the JSON refcache file. - Disabled HTML escaping when encoding URLs to JSON -- URLs are now stored in their unescaped form in `refcache.json` +- URLs stored in their unescaped form in `refcache.json` - Improves readability and prevents double-escaping issues ## Infrastructure & Tooling -### Version ID Suffix - -Added repository-specific suffix to version IDs for better build tracking. - -### CI/CD Improvements +### Status Code System -- Added `workflow_dispatch` trigger to GitHub Actions CI workflow -- Allows manual workflow runs from GitHub UI +Custom status code system for tool-specific states: -### Project Structure - -- Added `Makefile` for build automation -- Added `CONTRIBUTING.md` with development guidelines - -## Infrastructure & Tooling (continued) +- `htmltest/statuscodes.go` with constants and helpers +- Positive integers: HTTP status codes +- Zero: Unchecked/undiscovered links +- Negative integers: Tool-specific errors (timeout, network, cert, client) +- Helper functions: `IsHTTPStatus()`, `IsUnchecked()`, `IsToolError()` +- See `@docs/tasks/design.md` for conventions ### TDD Makefile Targets -**Status**: Completed - -Added Makefile targets for TDD workflow: +Makefile targets for TDD workflow: - `make test-tdd TEST_RUN=TestName` - Run test with clean cache +- `make test-tdd-fast` - Skip slow tests (timeouts) - `make test-tdd-cache TEST_RUN=TestName` - Run test and show cache state - `make clean-cache` - Remove refcache file - Supports pattern matching for running multiple tests ### Documentation Structure -**Status**: Completed - Organized documentation under `docs/`: -- `docs/ops/` - Operational documentation (session-start, agent-guidance) +- `docs/ops/` - Operational documentation (session-start) - `docs/tasks/` - Task and feature documentation - `AGENTS.md` - AI agent guidance (Cursor auto-loads) - `.github/copilot-instructions.md` - GitHub Copilot support -## Planned Features +### Project Structure -Most features are documented under the `docs/tasks/` directory. +- Added `Makefile` for build automation +- Added `CONTRIBUTING.md` with development guidelines +- Version ID suffix for better build tracking -### CacheAllExternal (In Progress) +### CI/CD -See `@docs/tasks/cache-unchecked-external-links.md` for details. This feature -will allow caching external links without checking them when -`CheckExternal: false`. +- Added `workflow_dispatch` trigger to GitHub Actions +- Updated action versions +- Prevents duplicate runs on PRs From 696db6408dcb3d5687ae6f97003b493a4f902c4c Mon Sep 17 00:00:00 2001 From: Patrice Chalin Date: Sun, 19 Oct 2025 11:29:20 -0400 Subject: [PATCH 14/14] Tidy up and fix lints --- go.mod | 2 +- htmltest/test_helpers_extra_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 17cae07..95091e4 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/golangplus/sort v1.0.0 // indirect github.com/imdario/mergo v0.3.11 github.com/seborama/govcr v4.5.0+incompatible // indirect - github.com/stretchr/testify v1.11.1 // indirect + github.com/stretchr/testify v1.11.1 golang.org/x/net v0.17.0 gopkg.in/seborama/govcr.v4 v4.5.0 gopkg.in/yaml.v2 v2.4.0 diff --git a/htmltest/test_helpers_extra_test.go b/htmltest/test_helpers_extra_test.go index de08a28..be48033 100644 --- a/htmltest/test_helpers_extra_test.go +++ b/htmltest/test_helpers_extra_test.go @@ -25,8 +25,8 @@ func tTestFileOptsFromCleanOutputDir(filename string, tOpts map[string]interface func tRemoveOutputDir(tOpts map[string]interface{}) { opts := DefaultOptions() - mergo.MergeWithOverwrite(&opts, tOpts) - os.RemoveAll(opts["OutputDir"].(string)) + _ = mergo.MergeWithOverwrite(&opts, tOpts) + _ = os.RemoveAll(opts["OutputDir"].(string)) } // Test skip helpers