# Batch 11 — Test Layer

**Scope:** All JS files under `architect-language-tools/test/` (24 files)
**Date:** 2026-03-26
**Reviewer:** Claude

---

## Files reviewed

### Infrastructure
- `test/__mocks__/vscode.js`
- `test/helpers/diagnosticHelpers.js`
- `test/helpers/mockDocument.js`
- `test/syntax-tests/runSyntaxSpecs.js`
- `test/syntax-tests/syntaxSpecs.test.js`
- `test/unit/validators/helpers.js`

### Unit tests
- `test/unit/codeActionProvider.test.js`
- `test/unit/completionProvider.test.js`
- `test/unit/definitionProvider.test.js`
- `test/unit/handleBindingAnalysis.test.js`
- `test/unit/includeResolver.test.js`
- `test/unit/parser.test.js`
- `test/unit/templateProvider.test.js`
- `test/unit/validators/functionValidator.test.js`
- `test/unit/validators/interpolationValidator.test.js`
- `test/unit/validators/keywordCapsValidator.test.js`
- `test/unit/validators/macroValidator.test.js`
- `test/unit/validators/namingValidator.test.js`
- `test/unit/validators/requireValidator.test.js`
- `test/unit/validators/robodocValidator.test.js`
- `test/unit/validators/typeKeywordCapsValidator.test.js`
- `test/unit/validators/typeValidator.test.js`
- `test/unit/validators/usagePatternValidator.test.js`
- `test/unit/validators/variableValidator.test.js`

---

## Findings

### Critical

None.

---

### Major

#### M1 — `functionValidator.test.js:29` — `.not.toThrow` property access instead of call — test silently passes without asserting anything

```js
// WRONG — property access on the return value of validateFunctionCalls()
expect(validateFunctionCalls(doc, alwaysWarn, { ... })).not.toThrow;

// CORRECT
expect(() => validateFunctionCalls(doc, alwaysWarn, { ... })).not.toThrow();
```

`expect(value).not.toThrow` (no call parentheses) accesses the `toThrow` property on the Jest matcher
object — it evaluates to a truthy non-null object but never executes an assertion. The test always
passes regardless of whether `validateFunctionCalls` throws or not.

This is compounded by the fact that `functionValidator` is the most complex validator and has only
3 test cases total, none of which exercise its core logic (function call validation, argument count
checking, undefined function detection). The test suite gives the false impression that this
validator is under test.

**Fix:** Change line 29 to `expect(() => validateFunctionCalls(doc, alwaysWarn, { ... })).not.toThrow()`.
Then add meaningful tests (see coverage gaps below).

#### M2 — `functionValidator.test.js` — critical validator has negligible coverage

`functionValidator.js` is the most feature-rich validator in the product (400+ lines). The test file
exercises only:
1. "No function calls in doc" (broken assertion — see M1)
2. Duplicate function name → ACU-FNC-003
3. Severity suppression

Completely absent:
- Calling an undeclared function → ACU-FNC-001
- Calling a function with wrong argument count → ACU-FNC-002
- Calling a function with unused return value (standalone call) → ACU-FNC-004
- Calling an included function (via mocked `buildIncludedFunctionsInScope`)
- Calling a built-in function — happy path
- Case-insensitive function name matching

The include resolver mock is already in place; these tests can be written without file I/O.

#### M3 — `typeKeywordCapsValidator.test.js` — Major bug from Batch 7 has no regression guard

The Batch 7 review identified that `currentPos` is not advanced for correctly-cased parameters in
multi-param FUNCTION declarations, causing all subsequent type error column offsets to be wrong.
No test exercises a FUNCTION with mixed-case params:

```
FUNCTION Calc(pName : STRING, pAge : long) : SHORT
```

Where `STRING` is correct but `long` is wrong. Without a test that checks the column offset of the
`long` diagnostic, the fix for this bug cannot be verified by the test suite.

**Fix:** Add a test checking `diags[0].range.start.character` after fixing the column-advance bug.

---

### Minor

#### m1 — `helpers/mockDocument.js:56` — `getWordRangeAtPosition` double-wraps RegExp argument

```js
const matches = [...line.matchAll(new RegExp(regex, 'g'))];
```

When `regex` is already a `RegExp` instance, `new RegExp(regex, 'g')` converts it to string first
(e.g., `/[A-Za-z_]+/i` → string `"/[A-Za-z_]+/i"`) and then wraps that string in a new RegExp,
producing a pattern that literally matches the slashes and flag characters. The test will not find
any word boundaries correctly.

No current test calls `getWordRangeAtPosition` with a RegExp argument, so this has not caused a
visible failure. It will silently produce wrong results if a test exercises that code path.

**Fix:** Use `regex instanceof RegExp ? new RegExp(regex.source, 'g') : new RegExp(regex, 'g')`.

#### m2 — `functionValidator.test.js:22-29` — first happy-path test structure is also wrong

Even if the `.not.toThrow` issue (M1) is fixed on line 29, the variable declaration on lines 22-28
calls `validateFunctionCalls(...)` directly (not inside a function wrapper), so the return value
is received by `expect()`, not a callable. The test block needs restructuring, not just adding `()`.

#### m3 — `namingValidator.test.js:97` — dead variable `vars`

```js
const vars = makeVarTypes([globalVar('zsName', 'STRING')]);   // declared but never used
// ...
const vars2 = makeVarTypes([globalVar('gsName', 'STRING')]);  // this is what's actually used
const diags = validateNamingConventions(doc, vars2, alwaysWarn);
```

`vars` is declared, never passed anywhere, and `vars2` is what's tested. The comment above also
tries to reason about `zs` prefix when the actual test uses `gs`. Misleading.

**Fix:** Remove `vars` and its comment; rename `vars2` to `vars`.

#### m4 — `templateProvider.test.js:15,52` — conflicting HTML tag case assertions

Line 15: `expect(template).toContain("<HTML>")` — uppercase.
Line 52: `expect(template).toContain("<html>")` — lowercase.

These are in separate `test()` blocks and both currently pass, which means the template contains
both `<HTML>` and `<html>`. This suggests inconsistently cased HTML in the generated template
output — not a test problem per se, but the tests are revealing a style inconsistency in the
product code.

#### m5 — `syntaxSpecs.test.js:14` — discovery guard threshold of 10 will silently degrade

```js
expect(summary.total).toBeGreaterThanOrEqual(10);
```

The spec count is currently much higher than 10. This guard was useful early on but will not catch
accidental deletion of spec files — it would need to drop below 10 to fail. Consider replacing with
a count that reflects the actual expected minimum, or remove it and rely on individual spec tests.

#### m6 — `runSyntaxSpecs.js:368` — column multiplier of 10000 undocumented and fragile

The edit sort comparator uses `line * 10000 + character` to produce a sortable integer. For lines
with more than 10,000 characters, edits on the same line could sort incorrectly. The magic number
is not explained.

**Fix:** Document the assumption, or use `{line, character}` tuple comparison directly.

#### m7 — `completionProvider.test.js` — only 4 tests, no edge cases

The `createMacroCompletionItems` function has no tests for:
- Position inside a quoted string (should not offer completions)
- Position inside a comment line
- No macros defined in document (should return empty)
- No typed prefix (all macros returned)
- Parameterised macro with multiple parameters (only single-param tested)

Also defines its own `createMockDocument` function, duplicating `test/helpers/mockDocument.js`.

#### m8 — `robodocValidator.test.js` — ACU-RBD-003 and `requiredSections` not tested

The test `enabledConfig()` configures:
- `validateHeaderTarget: true` — but no test verifies that a wrong header target triggers ACU-RBD-003
- `requiredSections: ['DESCRIPTION', 'CHANGE HISTORY']` — but no test verifies a doc missing a
  required section is flagged

The happy-path test at line 43 only verifies that a fully correct doc produces no diagnostics.

#### m9 — `macroValidator.test.js` — ACU-MAC-002 (case mismatch) not tested

ACU-MAC-002 fires when a macro is called with the wrong capitalisation (e.g., `init_report` when
defined as `INIT_REPORT`). The validator's `macroCaseMap` mechanism exists precisely to support
this check, but there is no test that exercises it.

#### m10 — `runSyntaxSpecs.js:417` — code action provider hardcoded to keyword capitalisation only

`testCodeActions` instantiates `new KeywordCapitalizationCodeActionProvider()` unconditionally.
Test specs that declare `code-action-tests` for other diagnostic types (e.g., type mismatch,
naming) will silently fail with "No code actions available" — there is no error indicating the
provider is wrong.

---

### Observations

#### O1 — `definitionProvider.test.js` — integration tests, not unit tests

Both tests read real files from `test/syntax-tests/included-files/` and exercise the full include
resolution stack. They are correctly identified as regression tests for circular-include handling,
but they provide no coverage of the common case: looking up a function defined in the same file, a
macro, a variable, or a position with no definition. The "unit" label is misleading.

#### O2 — `runSyntaxSpecs.js` matching logic has no self-tests

`containsDiagnostic`, `matchesDiagnosticById`, `normalizeExpectation`, and `getMessageExpectations`
are the heart of the syntax test harness — a bug here could silently cause all syntax tests to
pass regardless of actual diagnostic output. These functions have no dedicated tests.

#### O3 — `requireValidator.test.js` will need updating after requireValidator merge

The Batch 6 design recommendation is to merge `requireValidator.js` into `includeValidator.js`.
When that happens, `requireValidator.test.js` should be merged into or replaced by tests against
the combined validator.

#### O4 — Coverage gaps across the codebase (no tests at all for these modules)

The following production modules have no unit or integration test coverage:
- `core/src/collectDiagnostics.js` — orchestration; no test verifies validator wiring
- `core/src/suppressionParser.js` — `@rule-ignore` matching; noted as buggy in Batch 5
- `core/src/blockValidator.js` — block structure checking
- `core/src/diagnostics/diagnosticCatalog.js` — profile resolution logic
- `core/src/analysis/documentFacts.js` — cache hit/miss/eviction (noted in Batch 2)

#### O5 — `vscode.js` mock `CompletionItemKind` only defines `Function = 3`

Other `CompletionItemKind` values (`Text`, `Variable`, `Keyword`, `Snippet`, etc.) are absent.
Tests that assert on completion item kinds other than `Function` will receive `undefined` rather
than the expected integer, producing false-negative passes or false-positive failures depending on
what is asserted.

---

## Summary

| Severity | Count |
|---|---|
| Critical | 0 |
| Major | 3 |
| Minor | 10 |
| Observation | 5 |

The test layer is generally well-structured and the pattern (`makeDocument` + `alwaysWarn`/`alwaysOff`
+ `messageIds`) is consistent across all validator tests. The main concerns are:

1. **`functionValidator`** is severely under-tested relative to its complexity, and the existing
   "happy path" test silently does nothing (M1 + M2).
2. **No regression guard** for the `typeKeywordCapsValidator` column-offset bug (M3).
3. Several rules (`ACU-MAC-002`, `ACU-RBD-003`) are configured but never asserted.
4. Key orchestration and infrastructure modules (`collectDiagnostics`, `suppressionParser`,
   `blockValidator`, `diagnosticCatalog`) have zero coverage.
