diff --git a/cmd/lab/main.go b/cmd/lab/main.go index 7cc44b5..ad92747 100644 --- a/cmd/lab/main.go +++ b/cmd/lab/main.go @@ -71,12 +71,29 @@ const ( // Nothing outside this package holds these strings to the tree. The paths leg // of the invariants scan reads this repository's own documents, which is the // files at the root and everything under docs/, and a path named inside the -// runner is outside that subject by the leg's own reckoning. So the guard is -// the test beside this declaration and there is no second one. -var documentsAnOperatorIsOwed = []string{ - "NOTICE.md", - "LICENSE", - "docs/privacy.md", +// runner is outside that subject by the leg's own reckoning. So the guards are +// the tests beside this declaration and there are none anywhere else. +// +// A document has TWO SPELLINGS AND THE PARAGRAPH OWES BOTH, which is what +// issue #235 found. inTree is where a reader of a checkout looks. beside is the +// name the same file carries as a release asset, and it is the base of inTree +// because the release copies the three files in flat, beside the binaries, +// rather than inside a directory or an archive. +// +// Two of the three are the same word twice and the third is not. A paragraph +// naming only inTree sends somebody holding a download into a docs/ that is not +// in front of them; one naming only beside sends a reader of a checkout to a +// file at the root that is not there either. So the paragraph names both, and +// the tests below hold each spelling to the layout it is for. +type document struct { + inTree string + beside string +} + +var documentsAnOperatorIsOwed = []document{ + {inTree: "NOTICE.md", beside: "NOTICE.md"}, + {inTree: "LICENSE", beside: "LICENSE"}, + {inTree: "docs/privacy.md", beside: "privacy.md"}, } // documentsParagraph is the pointer the operator is owed, and it is one string @@ -85,8 +102,9 @@ var documentsAnOperatorIsOwed = []string{ // one of the three files moves - which is the same failure the paragraph itself // is written against, one level up. const documentsParagraph = `NOTICE.md says what this program is for, LICENSE carries the terms it is under, -and docs/privacy.md says what stays on the host. Reading them is on you; this -text only says where they are. +and privacy.md says what stays on the host. Those are the names beside a +downloaded binary; in a checkout the third one is docs/privacy.md. Reading them +is on you; this text only says where they are. ` const usage = `lab reads this repository and reports what it examined. diff --git a/cmd/lab/main_test.go b/cmd/lab/main_test.go index 06ac730..960d6d7 100644 --- a/cmd/lab/main_test.go +++ b/cmd/lab/main_test.go @@ -4,8 +4,10 @@ import ( "bytes" "errors" "os" + "path" "path/filepath" "runtime/debug" + "sort" "strings" "testing" "time" @@ -236,16 +238,26 @@ func TestTheDocumentedCodesAreTheNumbersTheRecordFixes(t *testing.T) { } // TestHelpNamesTheDocumentsAnOperatorIsOwed holds the last paragraph of the -// usage text to the three files it points at. An operator who reads that -// paragraph and then looks for one of the files is the reader this asserts for, -// and both halves of the walk they make are here: that the text names the file, -// and that the file is in the tree to be found. +// usage text to the three files it points at, in both of the layouts those +// files are read in. An operator who reads that paragraph and then looks for +// one of the files is the reader this asserts for, and the walk they make is +// made from a checkout or from a download rather than from one of the two. // -// The second half is the one that earns its place. A pointer in the runner is -// outside the subject of the invariants paths leg, which reads the files at the -// root and everything under docs/ and holds those to the paths they name. So a -// document deleted or moved under this paragraph reddens nothing anywhere else, -// and a binary would go on telling operators to read a file that is not there. +// So it asserts three things. That the text names the checkout spelling, that +// it names the download spelling, and that the checkout spelling is in this +// tree to be found. +// +// The tree half is the one that earns its place against the rest of the suite. +// A pointer in the runner is outside the subject of the invariants paths leg, +// which reads the files at the root and everything under docs/ and holds those +// to the paths they name. So a document deleted or moved under this paragraph +// reddens nothing anywhere else, and a binary would go on telling operators to +// read a file that is not there. +// +// The download half is the one issue #235 opened, and namesOnItsOwn is what +// makes it an assertion rather than a formality: docs/privacy.md contains +// privacy.md, so a containment test is satisfied by a paragraph naming only the +// checkout spelling and would have been green through the whole defect. // // What it does not judge is whether any of the three says what it should. That // is a reading of prose and no test here makes it. @@ -256,11 +268,13 @@ func TestHelpNamesTheDocumentsAnOperatorIsOwed(t *testing.T) { } for _, document := range documentsAnOperatorIsOwed { - if !strings.Contains(out.String(), document) { - t.Errorf("the help output does not name %s:\n%s", document, out.String()) + for _, spelling := range []string{document.inTree, document.beside} { + if !namesOnItsOwn(out.String(), spelling) { + t.Errorf("the help output does not name %s as a path in its own right:\n%s", spelling, out.String()) + } } - if _, err := os.Stat(filepath.Join("..", "..", filepath.FromSlash(document))); err != nil { - t.Errorf("the help output names %s and it is not in this tree: %v", document, err) + if _, err := os.Stat(filepath.Join("..", "..", filepath.FromSlash(document.inTree))); err != nil { + t.Errorf("the help output names %s and it is not in this tree: %v", document.inTree, err) } } } @@ -270,10 +284,11 @@ func TestHelpNamesTheDocumentsAnOperatorIsOwed(t *testing.T) { // reaches for one or the other and not reliably for both, so a paragraph that // only one of them prints reaches only half of them. // -// It asserts the same two things that test does. That the output names each -// document, and that each document it names is in this tree to be found - a -// binary pointing at a file somebody moved is worse than one pointing nowhere, -// because the reader follows it. +// It asserts the same three things that test does. That the output names each +// document in the spelling a checkout has for it, that it names each one in the +// spelling a download has for it, and that the checkout spelling is in this tree +// to be found - a binary pointing at a file somebody moved is worse than one +// pointing nowhere, because the reader follows it. func TestVersionNamesTheDocumentsAnOperatorIsOwed(t *testing.T) { var out, errOut bytes.Buffer if got := run([]string{"version"}, &out, &errOut, ordinary(realWalk)); got != exitClean { @@ -281,13 +296,155 @@ func TestVersionNamesTheDocumentsAnOperatorIsOwed(t *testing.T) { } for _, document := range documentsAnOperatorIsOwed { - if !strings.Contains(out.String(), document) { - t.Errorf("the version output does not name %s:\n%s", document, out.String()) + for _, spelling := range []string{document.inTree, document.beside} { + if !namesOnItsOwn(out.String(), spelling) { + t.Errorf("the version output does not name %s as a path in its own right:\n%s", spelling, out.String()) + } + } + if _, err := os.Stat(filepath.Join("..", "..", filepath.FromSlash(document.inTree))); err != nil { + t.Errorf("the version output names %s and it is not in this tree: %v", document.inTree, err) + } + } +} + +// TestTheDownloadSpellingIsTheNameTheReleasePublishes holds the two spellings +// to each other. The release copies the three files in flat beside the +// binaries, so the name an operator sees is the base of the path the tree +// carries, and a declaration writing anything else is a paragraph that points +// at nothing on either route. +// +// It is separate from the two output tests because it judges the declaration +// rather than the text: a second document added here with a mistyped download +// spelling is caught before anybody asks what the runner printed. +func TestTheDownloadSpellingIsTheNameTheReleasePublishes(t *testing.T) { + for _, document := range documentsAnOperatorIsOwed { + if want := path.Base(document.inTree); document.beside != want { + t.Errorf("%s is published beside a binary as %s, and this declaration calls it %s", document.inTree, want, document.beside) + } + } +} + +// TestTheReleaseCopiesTheDocumentsInFlat is the other end of that relation, and +// without it the base above is an assumption about a file this package never +// reads. The release workflow is what puts the three documents beside the +// binaries, and it is what decides the name they arrive under: copied into the +// assets directory itself, each one lands under its base name, which is the +// spelling the paragraph prints. +// +// WHAT IT READS IS THE ONE COPY THAT NAMES THE DOCUMENTS, and what it compares +// is the set of paths that copy carries against the set this package declares. +// A document dropped from the release, one added to it, a copy that preserved +// the directory, and a flag that changed the destination all move that set or +// take the line out of reach, and each of them makes the printed paragraph +// wrong in a way nothing else here would notice. +// +// WHAT IT CANNOT SEE. It reads the workflow file rather than a release, so it +// says what the next run of that workflow would publish and nothing about what +// any published release contains. And it finds the copy by shape, so an +// assemble step rewritten into a loop or split across two commands fails here +// with the documents still travelling correctly - which is a red gate asking +// for this test to be rewritten against the new shape, not a defect in the +// release. +func TestTheReleaseCopiesTheDocumentsInFlat(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("..", "..", ".github", "workflows", "release.yml")) + if err != nil { + t.Fatalf("reading the release workflow: %v", err) + } + + var want []string + for _, document := range documentsAnOperatorIsOwed { + want = append(want, document.inTree) + } + + got, ok := copiedIntoTheAssets(string(raw), want[0]) + if !ok { + t.Fatalf("no line of the release workflow copies %s into %q, so nothing here publishes the documents beside the binaries", want[0], "${assets}/") + } + + sort.Strings(got) + sorted := append([]string(nil), want...) + sort.Strings(sorted) + if strings.Join(got, " ") != strings.Join(sorted, " ") { + t.Errorf("the release copies %v into the assets directory and this package declares %v", got, sorted) + } +} + +// copiedIntoTheAssets returns the operands of the one cp in a workflow that +// names the given path and writes into the assets directory itself. The +// destination is matched as the assets directory and nothing under it, because +// a copy into a subdirectory of it publishes an asset under a name that is not +// the base of its path, which is the case the caller exists to refuse. +func copiedIntoTheAssets(workflow, naming string) ([]string, bool) { + for _, line := range strings.Split(workflow, "\n") { + fields := strings.Fields(strings.TrimSpace(line)) + if len(fields) < 3 || fields[0] != "cp" { + continue + } + if fields[len(fields)-1] != `"${assets}/"` { + continue + } + operands := fields[1 : len(fields)-1] + for _, operand := range operands { + if operand == naming { + return operands, true + } + } + } + return nil, false +} + +// namesOnItsOwn says whether text names a path in its own right rather than as +// the tail of a longer one. +// +// THIS IS THE WHOLE DIFFERENCE BETWEEN THE TWO SPELLINGS BEING ASSERTED AND +// NEITHER OF THEM BEING ASSERTED. docs/privacy.md ends with privacy.md, so +// strings.Contains cannot tell a paragraph naming both from one naming only the +// checkout spelling, and the paragraph issue #235 was raised against would +// satisfy it for every document in the list. +func namesOnItsOwn(text, name string) bool { + for _, token := range pathTokens(text) { + if token == name { + return true } - if _, err := os.Stat(filepath.Join("..", "..", filepath.FromSlash(document))); err != nil { - t.Errorf("the version output names %s and it is not in this tree: %v", document, err) + } + return false +} + +// pathTokens splits text into the maximal runs of bytes a path can be written +// with, and trims a trailing full stop from each, because a path at the end of +// a sentence is followed by one and the sentence is not part of the path. +// +// Ordinary words come out of it as tokens too, which costs nothing: the caller +// compares against a declared path, and no document in the list is spelled like +// a word of the paragraph around it. +func pathTokens(text string) []string { + var tokens []string + start := -1 + for i := 0; i <= len(text); i++ { + if i < len(text) && partOfAPath(text[i]) { + if start < 0 { + start = i + } + continue + } + if start >= 0 { + tokens = append(tokens, strings.TrimRight(text[start:i], ".")) + start = -1 } } + return tokens +} + +// partOfAPath says whether a byte can appear inside a path as this repository +// writes them. The set is deliberately wider than the three documents need, so +// that a neighbouring path in the same sentence is one token rather than +// several and cannot be mistaken for a document by accident. +func partOfAPath(b byte) bool { + switch { + case b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z', b >= '0' && b <= '9': + return true + } + return b == '/' || b == '\\' || b == '.' || b == '-' || b == '_' } // TestVersionReportsWhatTheToolchainStamped asserts that the version the verb