Add dot correctness tsts

.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 }}

.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

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

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

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=\"<<B X=\"1\">text</B>>\"] }").valid;
+false
+#@fi
+
+gap> STOP_TEST("graphviz package: dot-validation.tst", 0);

tst/dot.tst

@@ -203,14 +203,14 @@ gap> n := GraphvizAddNode(gv, 1);
 <graphviz node "1">
 gap> n := GraphvizSetAttr(n, "probably not ok", 1);
 <graphviz node "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", "<<>>");
 <graphviz node "1">
-gap> Print(AsString(gv));
+gap> Print(AsString(gv : invalidDOT := "unquoted attribute name with spaces"), "\n");
 //dot
 graph xxx {
 	1 [label="<<>>", probably not ok="1"]

tst/examples/structs.tst

@@ -63,7 +63,7 @@ gap> GraphvizAddEdge(s, "struct1:f2", "struct3:here");
 <graphviz edge (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=\"<<T\
 ABLE BORDER=\"0\" CELLBORDER=\"1\" CELLSPACING=\"0\">\n<TR>\n<TD>left</TD>\n<T\
 D PORT=\"f1\">middle</TD><TD PORT=\"f2\">right</TD>\n</TR>\n</TABLE>>\"]\n\tst\

tst/subgraph.tst

@@ -401,18 +401,18 @@ 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")));
-<graphviz graph "context+subgraph" with 0 nodes and 0 edges>
+<graphviz graph "context_and_subgraph" with 0 nodes and 0 edges>
 gap> legend := GraphvizAddContext(gv, "legend");
 <graphviz context "legend" with 0 nodes and 0 edges>
 gap> GraphvizSetAttrs(legend, rec(node := rec(shape := "plaintext")));
 <graphviz context "legend" with 0 nodes and 0 edges>
 gap> GraphvizAddSubgraph(legend, "legend");
 <graphviz graph "legend" with 0 nodes and 0 edges>
 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");;

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

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;