Add tests for guarantees L2, L7, L8

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"),

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)
+		}
+	}
+}

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)
+	}
+}

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)
+	}
+}

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)