test(tools): add unit tests for internal/ucd

[AhmadAL-Quraan]

Summary

Closes #21
Adds Go unit tests for tools/internal/ucd, which previously had no test
files (go test ./... reported "no test files"). This package parses the
Unicode Character Database (UCD) source files that back the committed
Kotlin lookup tables — a parsing bug here can silently corrupt generated
data downstream, so it's worth covering directly.

What's tested

• ucd_test.go — hex-scalar and code-point range parsing
  (ParseCodeRange, ScalarsToString), including surrogate and
  out-of-range rejection; Unicode version-pin parsing; repoRoot().
• mapping_test.go — parseLine (every IDNA status keyword,
  blank/comment lines, malformed input), loadRanges (gap detection,
  overlap detection, sort-before-validate), mergeAdjacent.
• validity_test.go — Mark/Virama classification from
  UnicodeData.txt rows, <..., First>/<..., Last> block expansion,
  Joining_Type parsing, range-merging helpers.
• nfc_test.go — combining-class and decomposition parsing,
  exclusion-set parsing, canonical-composition table construction
  (including the last-write-wins collision rule and non-starter-first
  rejection).
• bidi_test.go — Bidi_Class parsing, untracked-class filtering,
  First/Last block expansion.

Design note

tools/ reads real UCD source files that aren't committed to the repo
(per CLAUDE.md, only the generated tables are). So every test here
exercises the package's parsing functions directly against small,
hand-written byte fixtures instead of the real vendored corpus — this
keeps go test ./... hermetic and runnable by any contributor without
needing to fetch anything first.

Refactor

Extracted parseVersionPin(pin string) out of BundledUnicodeVersion()
(previously inlined against one hardcoded constant) so the version-pin
parser can be tested against a range of valid and malformed inputs.
No behavior change — BundledUnicodeVersion() still returns the same
value for the same input.

Testing

cd tools
go build ./...
go vet ./...
gofmt -l . # clean
go test ./internal/ucd/... -v # all passing
go test ./internal/ucd/... -cover # 70.1%

This PR only touches internal/ucd; internal/idnaref and
internal/codegen still have no test files and will follow in separate
PRs.

[OmarAlJarrah]

Thanks for this — it closes a real gap, and the hermetic-fixture decision is the right one. Fixtures use realistic full-width UCD rows rather than stripped-down synthetic lines, and several tests pin genuinely subtle invariants (deviation targets discarded, last-write-wins collision ordering in buildComposition, non-starter-first rejection, the bidiClasses set). The parseVersionPin extraction is clean and behaviour-preserving.

I verified the PR's claims against a checkout of the head commit: gofmt -l clean, go vet clean, all tests pass, coverage 70.1%. Coverage lands in the right place — the parsers are 90–100%, and the 30% shortfall is entirely the file-IO orchestrators (LoadTable/LoadNfc/LoadValidity/LoadBidi at 0%) plus the path helpers, i.e. exactly the parts that need the untracked corpus.

Inline comments below. Two things I'd want resolved before merge, one of which isn't anchorable to a file in this diff:

These tests never run. grep -riE "setup-go|go test|go-version|golang" .github/workflows/ returns zero hits across all five workflows. ci.yml has jobs for JVM, JS/Wasm, native, Android unit and Android instrumented — nothing for Go — and ./gradlew build doesn't invoke them either. So 981 lines of tests land as a manual-only artifact, and the first regression they'd catch gets merged silently. This is the highest-value thing to add here: a tools job in ci.yml using actions/setup-go with go-version-file: tools/go.mod, running go build ./... && go vet ./... && test -z "$(gofmt -l .)" && go test ./..., wired into the aggregate ci job's needs: list. It needs no JDK and no emulator, so it costs seconds; gating it on paths: tools/** keeps it off unrelated PRs.

The second is the mergeSetRanges test that asserts behaviour the function doesn't have — see the inline comment on validity_test.go.

A recurring theme in the rest: the suite repeatedly gets within one line of catching a real defect in the code under test. parseLine panics on a line with no ;, loadValidityUnicodeData panics on a short line where its two sibling loaders skip, and ScalarsToString folds negative input to U+FFFD despite a doc comment promising it can't. Each is pre-existing rather than introduced here, but closing them now is much cheaper than a follow-up, and in each case the missing test is a one-line variation on a test you already wrote.

[OmarAlJarrah]

Nice extraction. I checked this is genuinely behaviour-preserving: the only other change is threading pin into the three error messages in place of the constant, and BundledUnicodeVersion() still returns identical results for identical input. Splitting it is what makes the malformed-pin table below possible, and the doc comment explains why the split exists rather than restating what it does.

[OmarAlJarrah]

This checks the upper bound but not the lower one, and there's a real hole underneath it.

ScalarsToString's guard is code > maxScalar || (code >= firstSurrogate && code <= lastSurrogate) — no lower bound. strconv.ParseInt accepts a leading -, so a negative token sails past the guard and WriteRune(rune(-1)) silently emits U+FFFD:

ScalarsToString("-1") = "\ufffd" (runes [65533]), err = <nil>

That's exactly the failure mode the function's own doc comment promises against — "a token that is not valid hex, out of range, or a surrogate code point is returned as an error rather than silently skipped or folded to U+FFFD by WriteRune" — and exactly what this test exists to prove impossible.

Fix is one word in ucd.go: code < 0 || code > maxScalar. Then add "-1" (and maybe "-0041") alongside the 110000 case here.

[OmarAlJarrah]

This is subsumed by TestScalarsToStringRejectsSurrogateBoundaries immediately below, which tests D800 again plus DFFF/DC00 and both boundary-adjacent accepts. Worth folding this one into that test so there's a single place describing surrogate handling.

[OmarAlJarrah]

Hardcoding 17.0 / 17.0.0 is a defensible ratchet — a version bump should have to acknowledge this. The problem is that nothing will point the person doing the bump at it.

docs/idna-unicode-update.md step 5 says to grep for the outgoing version's Unicode <OLD> / unicode-<OLD> strings. Neither pattern matches the bare "17.0" string literals here, so the documented procedure walks straight past this file. Step 6 is ./gradlew build, which doesn't run Go tests — and per the summary comment, neither does CI. So a bump would leave a failing test that nothing runs and no step mentions.

Either add ucd_test.go to the step-5 list in that doc, or derive the expectation from unicodeVersionDir and assert only the shape here (patch defaults to 0, MajorMinor() is a prefix of String()), leaving the literal-version assertion to the doc's checklist.

[OmarAlJarrah]

Since you've established the t.TempDir() + t.Chdir pattern for repoRoot, joiningPath is worth the same treatment while you're here. It's the one path helper with real branching logic rather than a straight filepath.Join — it tries extracted/DerivedJoiningType.txt first and falls back to the top-level copy — and both arms plus the "neither exists" error are reachable by creating the files under a temp root. The other four path helpers aren't worth testing, but this one is.

[OmarAlJarrah]

This asserts behaviour that doesn't exist, and the test directly below it proves as much.

{0,5} and {6,10} are contiguous — 6 immediately follows 5, nothing is uncovered. There's no gap here at all. A real single-point gap does not merge:

mergeSetRanges({0,5},{7,10}) = [{start:0 end:5} {start:7 end:10}]   // 2 ranges, not 1

Three consequences:

1. This is a duplicate of the 21 == 20+1 "touches" case already covered by TestMergeSetRangesMergesTouchingAndOverlapping, relabelled as a distinct third behaviour.
2. It contradicts TestMergeSetRangesNoMergeWithGap directly below, whose {0,5},{7,10} fixture is the single-point-gap case and correctly asserts no merge.
3. It enshrines a wrong claim in a form that reads as verified-by-test, which is how this kind of thing gets cited later.

The comment is inherited from validity.go — "adjacency-merges any pair that touches, overlaps, or is separated by a single code point" — where the third clause is wrong too (or at best ambiguously worded). Worth fixing both: rename this to ...Touching... or drop it as redundant, and correct the source comment while you're in there.

[OmarAlJarrah]

mergeTypedRanges is the weakest-covered function that this PR does test, at 78.6%. What's unexercised is the sort comparator's equal-start and equal-end tiebreakers and the containment branch (current.end <= merged[last].end, where a range is fully inside the one before it). Every fixture here is two ranges with distinct starts, so the comparator never gets past its first if. One 4-element fixture with a couple of equal starts and one contained range would close most of it.

Separately, toPlain and toTyped in this file are at 0%. They're pure, dependency-free converters with no corpus requirement — a few lines each to cover, if you want the cheap points.

[OmarAlJarrah]

The assertion is weaker than the name suggests: "want both empty" would hold even if the 0041 record were dropped entirely, so it never confirms the real record was processed. What actually makes this test work is the implicit err != nil check above — if the blank line weren't skipped, ParseInt("") would error.

Compare TestLoadBidiUnicodeDataSkipsShortLines, which asserts len(got) == 1 and so pins both halves. A fixture whose real record contributes something (a non-zero CCC, or a canonical decomposition) would let you assert the same way here.

[OmarAlJarrah]

Good that invalid hex is covered. Worth deciding explicitly whether out-of-range and surrogate targets should be too — right now they're accepted silently:

loadNfcUnicodeData("00C0;X;Lu;0;L;110000 D800;;;;N;;;;;\n")
  decomposition = map[192:[1114112 55296]], err = <nil>

parseHexTokens validates nothing beyond hex syntax. That's arguably fine and mirrors Python's int(tok, 16), but ScalarsToString in the same package deliberately rejects both, so the two hex-list parsers disagree. Since the real corpus never contains such values, either answer works — I'd just rather it be a decision recorded in a test or comment than an accident.

[OmarAlJarrah]

This is the version the other two loaders' tests should look like: it asserts len(got) == 1, so it pins both that the blank line was skipped and that the real record survived. The NFC equivalent only asserts emptiness, and the validity equivalent doesn't exist — see the comments on those files.