diff --git a/cmd/create/parent_test.go b/cmd/create/parent_test.go index fccbfe2..d0e603c 100644 --- a/cmd/create/parent_test.go +++ b/cmd/create/parent_test.go @@ -31,6 +31,35 @@ func TestResolveParentNoneGiven(t *testing.T) { } } +// TestResolveParentIgnoresDirectoryNesting is guarantee L8 (no-layout-inference, +// docs/guarantees.md): hierarchy is never inferred from disk layout. The +// directory shape here is deliberately the one most tempting to "helpfully" +// infer from -- a file named after its parent directory, one level down from a +// same-named sibling file -- and it must still resolve to no parent at all +// when nothing said so. +func TestResolveParentIgnoresDirectoryNesting(t *testing.T) { + root := rootFor(t, t.TempDir()) + if err := os.MkdirAll(filepath.Join(root.Dir, "section"), 0o755); err != nil { + t.Fatal(err) + } + // "section.md" looks exactly like an index/parent for the "section/" the + // nested file lives under; neither has a parent: field. + if err := os.WriteFile(filepath.Join(root.Dir, "section.md"), + []byte("---\npage_id: 100\ntitle: Section\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + nested := filepath.Join(root.Dir, "section", "page.md") + + p, err := resolveParent(nested, map[string]string{}, nil, nil, "", root) + if err != nil { + t.Fatal(err) + } + if p.kind != parentTop { + t.Errorf("kind = %q, want top: nesting under a directory matching a sibling file's "+ + "name must not imply a parent", p.kind) + } +} + func TestResolveParentMdFileNotFound(t *testing.T) { root := rootFor(t, t.TempDir()) _, err := resolveParent(filepath.Join(root.Dir, "a.md"), diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go index 25c8d37..d62478d 100644 --- a/cmd/create/run_test.go +++ b/cmd/create/run_test.go @@ -459,3 +459,28 @@ func TestCreateAllStubIsEmptyThenPublished(t *testing.T) { t.Errorf("published body missing content: %q", p.body) } } + +// TestCreateBatchIgnoresDirectoryNesting is guarantee L8 (no-layout-inference, +// docs/guarantees.md) at the batch level: a file nested several directories +// deep, alongside files at shallower levels with names that could plausibly +// read as its ancestors, must still be created as a top-level page unless a +// parent: field or --parent said otherwise. Nothing about the tree shape may +// contribute to the decision. +func TestCreateBatchIgnoresDirectoryNesting(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + + shallow := write(t, dir, "docs.md", "---\ntitle: Docs\n---\nbody\n") + nested := write(t, dir, "docs/guide.md", "---\ntitle: Guide\n---\nbody\n") + deep := write(t, dir, "docs/guide/page.md", "---\ntitle: Page\n---\nbody\n") + + c, _ := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{shallow, nested, deep}) + for _, r := range ordered { + if r.parent.kind != parentTop { + t.Errorf("file %s: parent kind = %q, want top -- its directory depth must not "+ + "imply a parent relationship to the other files in this batch", r.filename, r.parent.kind) + } + } +} diff --git a/internal/convert/output_parses_test.go b/internal/convert/output_parses_test.go new file mode 100644 index 0000000..cc90679 --- /dev/null +++ b/internal/convert/output_parses_test.go @@ -0,0 +1,172 @@ +package convert_test + +// This file tests guarantee L7 (output-is-valid-markdown, docs/guarantees.md): +// anything markfluence writes to disk is markdown that renders. Every other +// test in this package checks that StorageToMarkdown produces a specific +// *string*; this one checks that the string it produces is actually +// recognized as the markdown it looks like, by feeding it back through a real +// parser rather than just eyeballing the golden. + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/convert" + "github.com/yuin/goldmark" + "github.com/yuin/goldmark/ast" + "github.com/yuin/goldmark/extension" + extast "github.com/yuin/goldmark/extension/ast" + "github.com/yuin/goldmark/text" +) + +// gfmForL7 is a plain GFM parser -- deliberately not the storageRenderer +// instance internal/convert uses to go the other way, since L7 is about +// whether a generic markdown consumer (a preview pane, GitHub, another tool) +// recognizes the output, not whether markfluence's own machinery can read it +// back. +var gfmForL7 = goldmark.New(goldmark.WithExtensions(extension.GFM)) + +// imageOrLinkRE approximates GFM's own bracket syntax well enough to count how +// many "![...](...)" / "[...](...)" occurrences the source *looks like it +// contains, as a floor: a real parser is allowed to recognize more (a +// reference-style link this regex doesn't match), never fewer. The +// destination alternatives mirror the one thing that turns real bracket +// syntax into inert literal text: a bare (non-angle-bracketed) destination +// containing a raw space is not a valid link/image at all, by GFM's own rule +// (see the "bare space" cases in doc-links-encoded and images-encoded-src) -- +// counting it here would be this test's own false positive, not a bug. +var imageOrLinkRE = regexp.MustCompile(`!?\[[^\]]*\]\((<[^>]*>|[^()\s]*)\)`) + +// tableSeparatorRE matches a GFM table header-separator row, e.g. "|---|---|" +// or "--- | :---:". Its presence is what commits the source to being parsed +// as a table rather than a paragraph of pipe characters. +var tableSeparatorRE = regexp.MustCompile(`(?m)^\s*\|?\s*:?-{3,}:?\s*(\|\s*:?-{3,}:?\s*)+\|?\s*$`) + +// fencedCodeRE matches a whole ``` ... ``` fenced block, so its contents can +// be excluded from checks that only make sense for prose. +var fencedCodeRE = regexp.MustCompile("(?s)```.*?```") + +// TestStorageToMarkdownOutputParsesAsMarkdown converts every storage2md case's +// input fresh -- the exact body read/export would write to disk -- through a +// real GFM parser, and confirms the bracket/table syntax markfluence emitted +// was actually recognized as such, not left as literal text a broken +// construct degrades to. Deliberately reconverts input.storage rather than +// reading the output.md golden: a golden is static, so reading it directly +// would check that one committed snapshot parses forever, regardless of +// whether the converter that produced it still does the same thing today. +func TestStorageToMarkdownOutputParsesAsMarkdown(t *testing.T) { + entries, err := os.ReadDir(storage2mdDir) + if err != nil { + t.Fatalf("reading %s: %v", storage2mdDir, err) + } + for _, e := range entries { + if !e.IsDir() || strings.HasPrefix(e.Name(), ".") { + continue + } + name := e.Name() + t.Run(name, func(t *testing.T) { + in, err := os.ReadFile(filepath.Join(storage2mdDir, name, "input.storage")) + if err != nil { + t.Fatalf("reading input: %v", err) + } + md, err := convert.StorageToMarkdown(string(in), convert.StorageOptions{}) + if err != nil { + t.Fatalf("StorageToMarkdown: %v", err) + } + assertParsesCleanly(t, []byte(md)) + }) + } +} + +// TestForwardCorpusOutputParsesAsMarkdown does the same check against the +// larger regression corpus: every forward golden's real, markfluence-emitted +// storage HTML, converted back to markdown fresh and parsed. +func TestForwardCorpusOutputParsesAsMarkdown(t *testing.T) { + entries, err := os.ReadDir(regressionDir) + if err != nil { + t.Fatalf("reading %s: %v", regressionDir, err) + } + for _, e := range entries { + if !e.IsDir() || strings.HasPrefix(e.Name(), ".") || strings.HasPrefix(e.Name(), "_") { + continue + } + name := e.Name() + t.Run(name, func(t *testing.T) { + data, err := os.ReadFile(filepath.Join(regressionDir, name, "test.output")) + if err != nil { + t.Fatalf("reading golden: %v", err) + } + var page struct { + HTML string `json:"html"` + } + if err := json.Unmarshal(data, &page); err != nil { + t.Fatalf("parsing golden: %v", err) + } + md, err := convert.StorageToMarkdown(page.HTML, convert.StorageOptions{}) + if err != nil { + t.Fatalf("StorageToMarkdown: %v", err) + } + assertParsesCleanly(t, []byte(md)) + }) + } +} + +// assertParsesCleanly is the shared check TestStorageToMarkdownOutputParsesAsMarkdown +// and cmd/read's/cmd/export's own tests can reuse: parse source and confirm the +// image/link/table syntax it appears to contain was actually recognized. +func assertParsesCleanly(t *testing.T, source []byte) { + t.Helper() + + doc := gfmForL7.Parser().Parse(text.NewReader(source)) + + var images, links, tables int + err := ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if !entering { + return ast.WalkContinue, nil + } + switch n.Kind() { + case ast.KindImage: + images++ + case ast.KindLink: + links++ + case extast.KindTable: + tables++ + } + return ast.WalkContinue, nil + }) + if err != nil { + t.Fatalf("walking parsed AST: %v", err) + } + + // prose excludes fenced code for every check below: example code inside + // one is free to contain bracket or table-separator-looking text (e.g. a + // snippet documenting markdown syntax) with no bearing on this guarantee, + // and counting it would be this test's own false positive, not a bug. + prose := fencedCodeRE.ReplaceAll(source, nil) + + wantAtLeast := len(imageOrLinkRE.FindAllIndex(prose, -1)) + if got := images + links; got < wantAtLeast { + t.Errorf("parsed %d image/link node(s), want at least %d matching the source's bracket syntax:\n%s", + got, wantAtLeast, source) + } + + if tableSeparatorRE.Match(prose) && tables == 0 { + t.Errorf("source has a table separator row but no Table node was parsed -- "+ + "it degraded to plain text:\n%s", source) + } + + // A structural sanity check independent of the floor comparison above, + // which can't see a rendering bug that mangles bracket syntax badly enough + // that the output no longer looks like link/image syntax at all -- nothing + // would be left for imageOrLinkRE to flag as missing. Every "[" markfluence + // emits outside a code fence closes, so an unequal count means something (a + // link, an alt-text bracket) was truncated or malformed outright. + if opens, closes := bytes.Count(prose, []byte("[")), bytes.Count(prose, []byte("]")); opens != closes { + t.Errorf("unbalanced brackets outside fenced code: %d '[' vs %d ']':\n%s", opens, closes, source) + } +} diff --git a/internal/project/cache_test.go b/internal/project/cache_test.go index 5fed55b..6ce8bcf 100644 --- a/internal/project/cache_test.go +++ b/internal/project/cache_test.go @@ -192,3 +192,66 @@ func TestCacheCloseIsSafeWithABackfilledSharedRoot(t *testing.T) { } c.Close() // must not panic despite closing the same *os.Root more than once } + +// TestCacheResolveIndependentOfBatchComposition is the other half of +// guarantee L2 (invocation-independent, docs/guarantees.md): resolving one +// file's root must not depend on which other files happen to be in the same +// batch. A single Cache is shared across every file in a create/update +// invocation, so its memoization must be purely an optimization -- it must +// never change what a given directory resolves to depending on what else was +// resolved through the same Cache, in either order. +// +// target and sibling deliberately belong to two different, unrelated +// projects (each with its own marker file): a contamination bug that just +// returns whatever the cache last resolved -- rather than what was actually +// asked for -- would otherwise happen to produce the right answer whenever a +// test's fixtures all shared one root, and pass by accident. +func TestCacheResolveIndependentOfBatchComposition(t *testing.T) { + projectA := t.TempDir() + if err := os.WriteFile(filepath.Join(projectA, Filename), []byte("# marker\n"), 0o644); err != nil { + t.Fatal(err) + } + target := filepath.Join(projectA, "docs") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + + projectB := t.TempDir() + if err := os.WriteFile(filepath.Join(projectB, Filename), []byte("# marker\n"), 0o644); err != nil { + t.Fatal(err) + } + sibling := filepath.Join(projectB, "docs") + if err := os.MkdirAll(sibling, 0o755); err != nil { + t.Fatal(err) + } + + alone := NewCache("") + defer alone.Close() + wantRoot, err := alone.Resolve(target) + if err != nil { + t.Fatal(err) + } + if wantRoot.Dir != projectA { + t.Fatalf("test fixture is wrong: target resolved to %q, want %q", wantRoot.Dir, projectA) + } + + // A second, independent Cache resolves an unrelated sibling from a + // different project first, then the same target -- simulating a batch + // that also happened to include a file from "sibling". The order and the + // extra file must not change target's result. + withSibling := NewCache("") + defer withSibling.Close() + if _, err := withSibling.Resolve(sibling); err != nil { + t.Fatal(err) + } + got, err := withSibling.Resolve(target) + if err != nil { + t.Fatal(err) + } + + if got.Dir != wantRoot.Dir || got.File != wantRoot.File { + t.Errorf("target resolved to Dir=%q File=%q alongside a sibling from a different project, "+ + "want Dir=%q File=%q (its result when resolved alone) -- batch composition must not matter", + got.Dir, got.File, wantRoot.Dir, wantRoot.File) + } +} diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 60344c4..3602192 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -36,6 +36,35 @@ func TestDiscoverFindsProjectFile(t *testing.T) { } } +// TestDiscoverIndependentOfWorkingDirectory is guarantee L2 +// (invocation-independent, docs/guarantees.md): how a reference resolves must +// depend only on the files on disk, not on the working directory the process +// happens to be running from. Discover takes startDir as an argument rather +// than consulting os.Getwd, so this pins that as a behavioral guarantee +// against a future regression, not just an implementation detail nobody +// checks. +func TestDiscoverIndependentOfWorkingDirectory(t *testing.T) { + root := t.TempDir() + writeProjectFile(t, root) + sub := filepath.Join(root, "team", "sub") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + elsewhere := t.TempDir() // a directory with no relation to root at all + + t.Chdir(elsewhere) + got, err := Discover(sub) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != root { + t.Errorf("Dir = %q, want %q -- resolving an absolute path must not be "+ + "affected by the process's unrelated working directory", got.Dir, root) + } +} + func TestDiscoverNearestAncestorWins(t *testing.T) { outer := t.TempDir() writeProjectFile(t, outer)