Address Review of the Reworked Unit Tests

Recompute the v4 recipe comparison once instead of per feature (148 s to 6 s) and compare bounding boxes as well as volumes; drop the incidental NaN loop from the unrepairable Voronoi case; prove the renamed preset is the one selected instead of checking a dirty flag that selecting clears; tell sparse from empty infill in the solid-shell test; plus smaller cleanups.

tests/fff_print/test_cooling.cpp

@@ -1,7 +1,7 @@
 #include <catch2/catch_all.hpp>
 
 #include <catch2/catch_test_macros.hpp>
-#include "catch2/catch_message.hpp"
+#include <catch2/catch_message.hpp>
 #include "test_helpers.hpp"
 
 #include <algorithm>
@@ -30,7 +30,6 @@ TEST_CASE("Fan is held off for the initial layers", "[Cooling]")
         { "fan_cooling_layer_time",       1000 }, // every layer is short enough to run the fan
         { "layer_height",                 0.2 },
         { "initial_layer_print_height",   0.2 },
-        { "layer_change_gcode",           ";TEST_LAYER_CHANGE" },
     });
 
     // Record, for each layer, whether any extrusion was printed with the part fan running.
@@ -40,7 +39,7 @@ TEST_CASE("Fan is held off for the initial layers", "[Cooling]")
     std::istringstream input(gcode);
     std::string line;
     while (std::getline(input, line)) {
-        if (line.rfind(";TEST_LAYER_CHANGE", 0) == 0) {
+        if (line.rfind(";LAYER_CHANGE", 0) == 0) {
             ++layer;
             extruded_with_fan.push_back(false);
         } else if (line.rfind("M107", 0) == 0) {

tests/fff_print/test_print.cpp

@@ -145,12 +145,21 @@ SCENARIO("Changing the number of solid shell layers does not make all surfaces i
 		        }
 		    return has_fill;
 		};
+        // True when the layer has a sparse fill surface, so a layer with no fill at all does not count.
+		auto has_sparse_infill = [&print](size_t obj_id, size_t layer_id) {
+		    const Layer &layer = *(print.objects().at(obj_id)->get_layer(static_cast<int>(layer_id)));
+		    for (const LayerRegion *region : layer.regions())
+		        for (const Surface &surface : region->fill_surfaces.surfaces)
+		            if (! surface.is_solid())
+		                return true;
+		    return false;
+		};
         print.process();
         // Precondition: 2 solid top layers (79, 78) and one solid bottom layer (0).
         REQUIRE(is_solid_infill(0,  0));
         REQUIRE(is_solid_infill(0, 79));
         REQUIRE(is_solid_infill(0, 78));
-        REQUIRE_FALSE(is_solid_infill(0, 77));
+        REQUIRE(has_sparse_infill(0, 77));
         WHEN("Model is re-sliced with top_shell_layers == 3") {
 			config.set("top_shell_layers", 3);
 			print.apply(model, config);
@@ -162,10 +171,10 @@ SCENARIO("Changing the number of solid shell layers does not make all surfaces i
                 CHECK(is_solid_infill(0, 79));
                 CHECK(is_solid_infill(0, 78));
                 CHECK(is_solid_infill(0, 77));
-                CHECK_FALSE(is_solid_infill(0, 76));
+                CHECK(has_sparse_infill(0, 76));
             }
             AND_THEN("Middle layers keep sparse infill") {
-                CHECK_FALSE(is_solid_infill(0, 40));
+                CHECK(has_sparse_infill(0, 40));
             }
         }
     }

tests/fff_print/test_trianglemesh.cpp

@@ -2,7 +2,7 @@
 
 #include <catch2/catch_test_macros.hpp>
 #include <catch2/matchers/catch_matchers_floating_point.hpp>
-#include "catch2/matchers/catch_matchers.hpp"
+#include <catch2/matchers/catch_matchers.hpp>
 #include "libslic3r/TriangleMesh.hpp"
 #include "libslic3r/TriangleMeshSlicer.hpp"
 #include "libslic3r/Point.hpp"

tests/libslic3r/test_caddocument.cpp

@@ -44,6 +44,7 @@
 
 #include <cmath>
 #include <cstring>
+#include <boost/filesystem/path.hpp>
 #include <fstream>
 #include <memory>
 #include <set>
@@ -774,7 +775,8 @@ TEST_CASE("entity constraints: tangent/midpoint/symmetric/angle", "[CadDocument]
 TEST_CASE("imported text glyphs all extrude without failing (charset sweep)", "[CadDocument]")
 {
     // The bundled font, from the source tree: PROFILES_DIR is <repo>/resources/profiles.
-    const std::string font = std::string(PROFILES_DIR) + "/../fonts/HarmonyOS_Sans_SC_Regular.ttf";
+    const std::string font =
+        (boost::filesystem::path(PROFILES_DIR).parent_path() / "fonts" / "HarmonyOS_Sans_SC_Regular.ttf").string();
     REQUIRE(std::ifstream(font).good());
 
     const std::string charset =
@@ -4547,9 +4549,10 @@ TEST_CASE("a v4 recipe keeps every field value", "[CadDocument]")
     }
 
     // --- Layer 2: the production v4 load rebuilds the same geometry as the in-memory tree ---
-    // The golden tree is serialization coverage and does not recompute as a whole, so the
-    // features are switched on one at a time and any the in-memory tree cannot build are left
-    // off in both. Every step that builds must give the loaded tree the same bodies.
+    // The golden tree is serialization coverage and does not recompute as a whole: these
+    // features fail on the geometry they get, so they are left off in both trees.
+    const std::set<std::string> unbuildable{"Fillet_Lat35", "Chamfer_Top2", "Draft_F3", "Shell_T1375",
+                                            "GoldenProject"};
     CadDocument exp_doc = make_golden_doc_v1();
     // Fields appended after v4 are not in the file, so a v4 project reads them as defaults
     // (a v4 thread, for one, is not major-nominal). The reference has to match that.
@@ -4566,27 +4569,28 @@ TEST_CASE("a v4 recipe keeps every field value", "[CadDocument]")
     CadDocument doc;
     doc.deserialize_recipe(blob);
     REQUIRE(doc.features.size() == exp_doc.features.size());
-    for (CadFeature& f : exp_doc.features) f.enabled = false;
-    for (CadFeature& f : doc.features) f.enabled = false;
-    size_t built_steps = 0;
-    for (size_t i = 0; i < exp_doc.features.size(); ++i) {
-        exp_doc.features[i].enabled = doc.features[i].enabled = true;
-        if (!exp_doc.recompute()) {
-            exp_doc.features[i].enabled = doc.features[i].enabled = false;
-            continue;
+    for (size_t i = 0; i < exp_doc.features.size(); ++i)
+        exp_doc.features[i].enabled = doc.features[i].enabled = unbuildable.count(exp_doc.features[i].name) == 0;
+    const bool exp_built = exp_doc.recompute();
+    INFO("reference error: " << exp_doc.error);
+    REQUIRE(exp_built);
+    const bool built = doc.recompute();
+    INFO("loaded error: " << doc.error);
+    REQUIRE(built);
+    REQUIRE_FALSE(exp_doc.bodies.empty());
+    REQUIRE(doc.bodies.size() == exp_doc.bodies.size());
+    for (size_t b = 0; b < doc.bodies.size(); ++b) {
+        INFO("body " << b);
+        TriangleMesh mesh     = SketchEngine::tessellate(doc.bodies[b].shape);
+        TriangleMesh exp_mesh = SketchEngine::tessellate(exp_doc.bodies[b].shape);
+        CHECK_THAT(double(mesh.volume()), WithinRel(double(exp_mesh.volume()), 1e-6));
+        // Volume alone is blind to fields that only place a body, such as a transform.
+        const BoundingBoxf3 bb = mesh.bounding_box(), exp_bb = exp_mesh.bounding_box();
+        for (int k = 0; k < 3; ++k) {
+            CHECK_THAT(bb.min[k], WithinAbs(exp_bb.min[k], 1e-6));
+            CHECK_THAT(bb.max[k], WithinAbs(exp_bb.max[k], 1e-6));
         }
-        INFO("after feature " << i << " (" << exp_doc.features[i].name << ")");
-        REQUIRE(doc.recompute());
-        REQUIRE(doc.bodies.size() == exp_doc.bodies.size());
-        for (size_t b = 0; b < doc.bodies.size(); ++b) {
-            double v = double(SketchEngine::tessellate(doc.bodies[b].shape).volume());
-            double ev = double(SketchEngine::tessellate(exp_doc.bodies[b].shape).volume());
-            REQUIRE_THAT(v, WithinRel(ev, 1e-6));
-        }
-        ++built_steps;
     }
-    CHECK(built_steps > 0);
-    CHECK_FALSE(exp_doc.bodies.empty());
 }
 
 // --- Rib tests (M5b) ---

tests/libslic3r/test_preset_bundle_loading.cpp

@@ -1334,15 +1334,14 @@ TEST_CASE("Renamed printer/process names are normalized into compatible lists on
     CHECK(compatible_list(bundle.filaments, "My Filament", "compatible_printers") ==
           (std::vector<std::string>{ "New Printer", "Unknown Printer" }));
 
-    // Selecting the normalized preset copies the rewritten list into the edited preset, so it is
-    // not reported modified.
-    REQUIRE(bundle.prints.select_preset_by_name("My Process", true));
+    // Normalization runs before selection, so selecting copies the rewritten list into the edited preset.
+    bundle.prints.select_preset_by_name("My Process", true);
+    REQUIRE(bundle.prints.get_selected_preset_name() == "My Process");
     CHECK(bundle.prints.get_edited_preset().config.option<ConfigOptionStrings>("compatible_printers")->values ==
           std::vector<std::string>{ "New Printer" });
-    CHECK_FALSE(bundle.prints.current_is_dirty());
 
-    // A preset that already references the current name is left untouched (idempotent no-op), so
-    // the selection stays unmodified.
+    // Normalizing again is a no-op: the stored preset keeps the current name and still matches the
+    // edited copy, so the selection is not reported modified.
     bundle.normalize_compatible_presets();
     CHECK(compatible_list(bundle.prints, "My Process", "compatible_printers") == std::vector<std::string>{ "New Printer" });
     CHECK_FALSE(bundle.prints.current_is_dirty());
@@ -1375,10 +1374,13 @@ TEST_CASE("Renamed names are normalized into a SYSTEM preset's compatible lists"
     CHECK(compatible_list(bundle.filaments, "System Filament", "compatible_prints") ==
           std::vector<std::string>{ "New Process" });
 
-    // Selecting the normalized system preset does not report it modified, and the rewrite is
-    // idempotent.
-    REQUIRE(bundle.filaments.select_preset_by_name("System Filament", true));
-    CHECK_FALSE(bundle.filaments.current_is_dirty());
+    // Normalization runs before selection, so selecting copies the rewritten lists into the edited preset.
+    bundle.filaments.select_preset_by_name("System Filament", true);
+    REQUIRE(bundle.filaments.get_selected_preset_name() == "System Filament");
+    CHECK(bundle.filaments.get_edited_preset().config.option<ConfigOptionStrings>("compatible_printers")->values ==
+          std::vector<std::string>{ "New Printer" });
+
+    // Normalizing again is a no-op, so the selection is not reported modified.
     bundle.normalize_compatible_presets();
     CHECK(compatible_list(bundle.filaments, "System Filament", "compatible_printers") ==
           std::vector<std::string>{ "New Printer" });

tests/libslic3r/test_sketchimport.cpp

@@ -7,6 +7,7 @@
 #include "libslic3r/CAD/SketchImport.hpp"
 #include "test_utils.hpp"        // ScopedTemporaryFile
 
+#include <boost/filesystem/path.hpp>
 #include <cstdlib>
 #include <fstream>
 #include "libslic3r/Point.hpp"
@@ -78,7 +79,8 @@ TEST_CASE("transform_regions moves and scales independently", "[SketchImport]")
 TEST_CASE("text_to_regions vectorizes glyphs with counters", "[SketchImport]")
 {
     // The bundled font, from the source tree: PROFILES_DIR is <repo>/resources/profiles.
-    const std::string font = std::string(PROFILES_DIR) + "/../fonts/HarmonyOS_Sans_SC_Regular.ttf";
+    const std::string font =
+        (boost::filesystem::path(PROFILES_DIR).parent_path() / "fonts" / "HarmonyOS_Sans_SC_Regular.ttf").string();
     REQUIRE(std::ifstream(font).good());
 
     // Bad input is rejected without throwing.

tests/libslic3r/test_sketchprofile.cpp

@@ -113,16 +113,14 @@ TEST_CASE("profile: offsetting a closed rectangle keeps it closed", "[SketchProf
     REQUIRE(closed_wires(out) == 1);
 }
 
-TEST_CASE("profile: offset outward grows the enclosed area by the right amount", "[SketchProfile]")
+TEST_CASE("profile: offsetting a closed rectangle outward keeps it one closed loop", "[SketchProfile]")
 {
-    // Outward is -d for a CCW loop. The miter join keeps the corners sharp, so a rectangle
-    // offset outward by d is one closed (w+2d) x (h+2d) loop, not four disconnected segments.
-    const double w = 40, h = 20, d = 5;
-    const auto out = SketchEngine::offset_entities(rect(w, h), -d);
+    // Outward is -d for a CCW loop. The miter join reconnects the corners, so the result is one
+    // closed wire, not four disconnected segments. Its area is checked by the sign test below.
+    const auto out = SketchEngine::offset_entities(rect(40, 20), -5.0);
     const auto ws  = SketchEngine::entities_to_wires(out, xy_plane());
     REQUIRE(ws.size() == 1);
     REQUIRE(ws[0].Closed());
-    REQUIRE_THAT(profile_area(out), WithinAbs((w + 2 * d) * (h + 2 * d), 1e-6));
 }
 
 TEST_CASE("profile: offset sign is left-of-travel, so +d shrinks a CCW rectangle", "[SketchProfile]")

tests/libslic3r/test_voronoi.cpp

@@ -32,6 +32,7 @@
 using namespace Slic3r;
 
 using VD = Geometry::VoronoiDiagram;
+static bool has_missing_voronoi_vertices(const Polygons &polygons, const VD &vd);
 using BoostVD = boost::polygon::voronoi_diagram<double>;
 
 // https://svn.boost.org/trac10/ticket/12067
@@ -203,6 +204,7 @@ TEST_CASE("Voronoi missing edges - Alessandro gapfill 12707", "[Voronoi]")
     // Orca's detector finds nothing wrong with this input, so no repair is needed.
     CHECK(VD::detect_known_issues(vd, lines.begin(), lines.end()) == VD::IssueType::NO_ISSUE_DETECTED);
     CHECK(vd.get_state() == VD::State::REPAIR_NOT_NEEDED);
+    CHECK_FALSE(has_missing_voronoi_vertices({ poly }, vd));
 
 #ifdef VORONOI_DEBUG_OUT
     dump_voronoi_to_svg(debug_out_path("voronoi-lines.svg").c_str(),
@@ -1338,21 +1340,12 @@ TEST_CASE("Voronoi NaN coordinates 12139", "[Voronoi]")
 
     VD vd;
     vd.construct_voronoi(lines.begin(), lines.end());
-    // The input self-intersects, so rotation cannot repair the diagram. It must be reported as invalid,
-    // which is what MedialAxis::build checks before falling back to a morphological closing.
+    // The diagram built from this self-intersecting input has non-finite vertices, and rotating the
+    // input does not repair it. It must be reported as invalid, which is what MedialAxis::build
+    // checks before falling back to a morphological closing.
     CHECK(vd.get_state() == VD::State::REPAIR_UNSUCCESSFUL);
     CHECK_FALSE(vd.is_valid());
 
-    for (const auto& edge : vd.edges())
-        if (edge.is_finite()) {
-            auto v0 = edge.vertex0();
-            auto v1 = edge.vertex1();
-            REQUIRE((v0->x() == 0 || std::isnormal(v0->x())));
-            REQUIRE((v0->y() == 0 || std::isnormal(v0->y())));
-            REQUIRE((v1->x() == 0 || std::isnormal(v1->x())));
-            REQUIRE((v1->y() == 0 || std::isnormal(v1->y())));
-        }
-
 #ifdef VORONOI_DEBUG_OUT
     dump_voronoi_to_svg(debug_out_path("voronoi-NaNs.svg").c_str(),
         vd, Points(), lines, Polygons(), Lines(), 0.015);

tests/slic3rutils/test_plugin_capability_config.cpp

@@ -8,7 +8,7 @@
 #include <slic3r/plugin/PythonPluginInterface.hpp>
 
 #include <catch2/catch_test_macros.hpp>
-#include "catch2/catch_message.hpp"
+#include <catch2/catch_message.hpp>
 #include "plugin_test_utils.hpp"
 #include <pybind11/pytypes.h>
 #include <pybind11/eval.h>
@@ -158,9 +158,9 @@ class RestorePackage(orca.base):
         orca.register_capability(Raising)
 )PY";
 
-// Writes the restore fixture into {data_dir}/orca_plugins and loads it. Call without holding the GIL:
-// the load runs on a worker thread.
-bool load_restore_plugin(const ScopedDataDir& data_dir_guard)
+// Writes the restore fixture into {data_dir}/orca_plugins and loads it, returning the load error
+// (empty on success). Call without holding the GIL: the load runs on a worker thread.
+std::string load_restore_plugin(const ScopedDataDir& data_dir_guard)
 {
     const boost::filesystem::path plugin_dir = data_dir_guard.plugins_dir() / RESTORE_PLUGIN_KEY;
     boost::filesystem::create_directories(plugin_dir);
@@ -173,9 +173,9 @@ bool load_restore_plugin(const ScopedDataDir& data_dir_guard)
     manager.discover_plugins(/*async=*/false, /*clear=*/true);
     manager.load_plugin(RESTORE_PLUGIN_KEY, /*skip_deps=*/true);
     std::string error;
-    const bool  loaded = manager.wait_for_plugin_load(RESTORE_PLUGIN_KEY, std::chrono::seconds(120), error);
-    INFO("load error: " << error);
-    return loaded && error.empty();
+    if (!manager.wait_for_plugin_load(RESTORE_PLUGIN_KEY, std::chrono::seconds(120), error) && error.empty())
+        error = "the plugin did not load";
+    return error;
 }
 
 } // namespace
@@ -433,7 +433,9 @@ TEST_CASE("Restoring defaults overwrites only the target capability", "[PluginCo
 
     ScopedDataDir data_dir_guard("plugin-config-py-restore");
     host_config().load();
-    REQUIRE(load_restore_plugin(data_dir_guard));
+    const std::string load_error = load_restore_plugin(data_dir_guard);
+    INFO("load error: " << load_error);
+    REQUIRE(load_error.empty());
 
     const PluginCapabilityId target    = capability_id(PluginCapabilityType::Script, "Target", RESTORE_PLUGIN_KEY);
     const PluginCapabilityId bystander = capability_id(PluginCapabilityType::Script, "Bystander", RESTORE_PLUGIN_KEY);
@@ -459,7 +461,9 @@ TEST_CASE("A raising get_default_config leaves the stored config untouched", "[P
 
     ScopedDataDir data_dir_guard("plugin-config-py-restore-raise");
     host_config().load();
-    REQUIRE(load_restore_plugin(data_dir_guard));
+    const std::string load_error = load_restore_plugin(data_dir_guard);
+    INFO("load error: " << load_error);
+    REQUIRE(load_error.empty());
 
     const PluginCapabilityId raising = capability_id(PluginCapabilityType::Script, "Raising", RESTORE_PLUGIN_KEY);
     REQUIRE(host_config().store_capability_config(raising, json{{"keep", "me"}}));