diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 4f81314..39148ff 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -31,6 +31,11 @@ jobs: steps: - uses: actions/checkout@v7 + - name: Install Graphviz for DOT validation + run: | + sudo apt-get update + sudo apt-get install -y graphviz + dot -V - uses: gap-actions/setup-gap@v3 with: gap-version: ${{ matrix.gap-version }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b16e40f..2901680 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -24,4 +24,4 @@ jobs: run: pip install gaplint - name: "Run gaplint on the graphviz package" run: | - gaplint gap/*.g* tst/*.tst examples/*.g* PackageInfo.g init.g read.g makedoc.g tst/testall.g + gaplint gap/*.g* tst/*.tst examples/*.g* PackageInfo.g init.g read.g makedoc.g tst/*.g diff --git a/README.md b/README.md index bd7ff7e..ebea7c9 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,20 @@ Render and view the result: ![The Round Table](./doc/img/The_Round_Table.png) +## Testing + +Run `make check` (or `make check GAP=/path/to/gap`). When Graphviz's `dot` +is on `PATH`, the suite also parses every generated `AsString` result using +`dot -Tcanon`. Validation failures report the DOT and Graphviz diagnostics +and fail the suite even if the saved output matches. CI installs Graphviz; +local runs without it explicitly report that validation is skipped. + +Tests which intentionally produce invalid DOT must give a reason, for example +`AsString(gv : invalidDOT := "unquoted attribute name with spaces")`. +These cases must be rejected by Graphviz. This checks syntax, not layout or +whether the parsed graph has the intended meaning; the output comparisons +remain in place. + ## Issues For questions, remarks, suggestions, and issues please use the diff --git a/etc/gaplint.sh b/etc/gaplint.sh index 0d8af69..8cd0fb3 100755 --- a/etc/gaplint.sh +++ b/etc/gaplint.sh @@ -1,4 +1,4 @@ #!/bin/bash set -e -gaplint --disable W004 $@ *.g gap/* read.g init.g PackageInfo.g makedoc.g tst/testall.g tst/*.tst tst/examples/*.tst +gaplint --disable W004 $@ *.g gap/* read.g init.g PackageInfo.g makedoc.g tst/*.g tst/*.tst tst/examples/*.tst diff --git a/tst/dot-validation.tst b/tst/dot-validation.tst new file mode 100644 index 0000000..b9932e6 --- /dev/null +++ b/tst/dot-validation.tst @@ -0,0 +1,29 @@ +gap> START_TEST("graphviz package: dot-validation.tst"); + +# The validator is installed by testall.g when Graphviz is available. +#@if IsBound(GV_TEST_DOT) +gap> GV_TEST_DOT.Validate("graph { a -- b }").valid; +true +gap> GV_TEST_DOT.Validate("digraph { a -> b }").valid; +true +gap> GV_TEST_DOT.Validate("graph { }").valid; +true + +# Graphviz accepts empty input with status 0; require an actual parsed graph. +gap> GV_TEST_DOT.Validate("").valid; +false +gap> GV_TEST_DOT.Validate("// only a comment\n").valid; +false +gap> GV_TEST_DOT.Validate("digraph { a -- b }").valid; +false +gap> GV_TEST_DOT.Validate("graph { a -- b").valid; +false +gap> GV_TEST_DOT.Validate("graph { a [bad name=1] }").valid; +false +gap> GV_TEST_DOT.Validate("graph context+subgraph { }").valid; +false +gap> GV_TEST_DOT.Validate("graph { a [label=\"<text>\"] }").valid; +false +#@fi + +gap> STOP_TEST("graphviz package: dot-validation.tst", 0); diff --git a/tst/dot.tst b/tst/dot.tst index 6acb1f5..b2e32a8 100644 --- a/tst/dot.tst +++ b/tst/dot.tst @@ -203,14 +203,14 @@ gap> n := GraphvizAddNode(gv, 1); gap> n := GraphvizSetAttr(n, "probably not ok", 1); -gap> Print(AsString(gv)); +gap> Print(AsString(gv : invalidDOT := "unquoted attribute name with spaces"), "\n"); //dot graph xxx { 1 [probably not ok="1"] } gap> GraphvizSetAttr(n, "label", "<<>>"); -gap> Print(AsString(gv)); +gap> Print(AsString(gv : invalidDOT := "unquoted attribute name with spaces"), "\n"); //dot graph xxx { 1 [label="<<>>", probably not ok="1"] diff --git a/tst/examples/structs.tst b/tst/examples/structs.tst index 7ac00d5..94eb987 100644 --- a/tst/examples/structs.tst +++ b/tst/examples/structs.tst @@ -63,7 +63,7 @@ gap> GraphvizAddEdge(s, "struct1:f2", "struct3:here"); # -gap> AsString(s); +gap> AsString(s : invalidDOT := "inappropriate addition of quotes to HTML labels FIXME"); "//dot\ndigraph structs {\nnode [shape=\"plaintext\"]\n\tstruct1 [label=\"<\n\nleft\nmiddleright\n\n>\"]\n\tst\ diff --git a/tst/subgraph.tst b/tst/subgraph.tst index e2ca8ba..ba19045 100644 --- a/tst/subgraph.tst +++ b/tst/subgraph.tst @@ -401,9 +401,9 @@ gap> IsIdenticalObj(o, s); true # Test a context containing a subgraph -gap> gv := GraphvizGraph("context+subgraph");; +gap> gv := GraphvizGraph("context_and_subgraph");; gap> GraphvizSetAttrs(gv, rec(node := rec(shape := "box"))); - + gap> legend := GraphvizAddContext(gv, "legend"); gap> GraphvizSetAttrs(legend, rec(node := rec(shape := "plaintext"))); @@ -411,8 +411,8 @@ gap> GraphvizSetAttrs(legend, rec(node := rec(shape := "plaintext"))); gap> GraphvizAddSubgraph(legend, "legend"); gap> AsString(gv); -"//dot\ngraph context+subgraph {\nnode [shape=\"box\"]\n// legend context \n{\ -\nnode [shape=\"plaintext\"]\nsubgraph legend {\n}\n}\n}\n" +"//dot\ngraph context_and_subgraph {\nnode [shape=\"box\"]\n// legend context\ + \n{\nnode [shape=\"plaintext\"]\nsubgraph legend {\n}\n}\n}\n" # Test a context containing a subdigraph gap> gv := GraphvizDigraph("context+subgraph");; diff --git a/tst/testall.g b/tst/testall.g index 1401292..9a35565 100644 --- a/tst/testall.g +++ b/tst/testall.g @@ -2,12 +2,18 @@ # in PackageInfo.g. LoadPackage("GraphvizForGAP"); +GapExitCode(1); # compute packages not loaded and thus tests not run # todo: check if loading other packages without erroring if it fails is possible excl := []; +testOptions := rec(compareFunction := "uptowhitespace"); if Filename(DirectoriesSystemPrograms(), "dot") = fail then Add(excl, "splash.tst"); + Print("Skipping DOT validation: Graphviz program 'dot' not found\n"); +else + ReadPackage("GraphvizForGAP", "tst/testdot.g"); + testOptions.compareFunction := GV_TEST_DOT.Compare; fi; if Filename(DirectoriesSystemPrograms(), "pdflatex") = fail then Add(excl, "splash_latex.tst"); @@ -17,7 +23,17 @@ if not IsEmpty(excl) then Print("Excluding test files: ", excl, "\n"); fi; -TestDirectory(DirectoriesPackageLibrary("GraphvizForGAP", "tst"), - rec(exitGAP := true, compareFunction := "uptowhitespace", exclude := excl)); +success := TestDirectory(DirectoriesPackageLibrary("GraphvizForGAP", "tst"), + rec(testOptions := testOptions, exclude := excl)); + +if IsBound(GV_TEST_DOT) then + Print("DOT validation: ", GV_TEST_DOT.checked, " strings checked, ", + GV_TEST_DOT.invalid, " intentionally invalid, ", + GV_TEST_DOT.failures, " failures\n"); + success := success and GV_TEST_DOT.failures = 0; +fi; +if success then + FORCE_QUIT_GAP(0); +fi; FORCE_QUIT_GAP(1); # if we ever get here, there was an error diff --git a/tst/testdot.g b/tst/testdot.g new file mode 100644 index 0000000..ba21aa7 --- /dev/null +++ b/tst/testdot.g @@ -0,0 +1,69 @@ +# Test-only instrumentation: validate the actual strings returned by AsString. +if not IsBound(GV_TEST_DOT) then + BindGlobal("GV_TEST_DOT", rec( + dot := Filename(DirectoriesSystemPrograms(), "dot"), + directory := DirectoryTemporary(), + originalAsString := ApplicableMethod(AsString, [GraphvizGraph()]), + pending := [], + checked := 0, + invalid := 0, + failures := 0)); + + GV_TEST_DOT.Validate := function(source) + local canonical, input, output, diagnostics, status, parsed; + canonical := Filename(GV_TEST_DOT.directory, "canonical.dot"); + diagnostics := ""; + input := InputTextString(source); + output := OutputTextString(diagnostics, false); + # Pass paths as arguments; only the fixed redirection needs a shell. + status := Process(GV_TEST_DOT.directory, + Filename(DirectoriesSystemPrograms(), "sh"), + input, output, + ["-c", "exec \"$1\" -Tcanon -o \"$2\" 2>&1", "sh", + GV_TEST_DOT.dot, canonical]); + CloseStream(input); + CloseStream(output); + parsed := IsReadableFile(canonical) and not IsEmpty(StringFile(canonical)); + RemoveFile(canonical); + return rec(valid := status = 0 and parsed, + status := status, diagnostics := diagnostics); + end; + + InstallMethod(AsString, "validate DOT during package tests", + [IsGraphvizGraphDigraphOrContext], 100, + function(graph) + local entry; + entry := rec(source := GV_TEST_DOT.originalAsString(graph), + reason := ValueOption("invalidDOT")); + Add(GV_TEST_DOT.pending, entry); + return entry.source; + end); + + GV_TEST_DOT.Compare := function(expected, actual) + local ok, entry, result, invalid; + ok := true; + for entry in GV_TEST_DOT.pending do + result := GV_TEST_DOT.Validate(entry.source); + invalid := IsString(entry.reason) and not IsEmpty(entry.reason); + GV_TEST_DOT.checked := GV_TEST_DOT.checked + 1; + if invalid then + GV_TEST_DOT.invalid := GV_TEST_DOT.invalid + 1; + fi; + # Exit status 1 denotes rejected DOT, not a missing/crashed executable. + if (invalid and (result.valid or result.status <> 1)) + or (not invalid and not result.valid) then + ok := false; + GV_TEST_DOT.failures := GV_TEST_DOT.failures + 1; + if invalid then + Print("DOT validation: expected invalid DOT (", entry.reason, ")\n"); + else + Print("DOT validation: expected valid DOT\n"); + fi; + Print("Graphviz exit status: ", result.status, "\n", + result.diagnostics, "Generated DOT:\n", entry.source, "\n"); + fi; + od; + GV_TEST_DOT.pending := []; + return ok and TEST.compareFunctions.uptowhitespace(expected, actual); + end; +fi;