From 777f14fad4da8d91d162be0cd4092db71b023517 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Mon, 19 Oct 2026 17:57:04 +0800 Subject: [PATCH] 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 | 5 +- tests/fff_print/test_print.cpp | 15 ++++-- tests/fff_print/test_trianglemesh.cpp | 2 +- tests/libslic3r/test_caddocument.cpp | 50 ++++++++++--------- .../libslic3r/test_preset_bundle_loading.cpp | 22 ++++---- tests/libslic3r/test_sketchimport.cpp | 4 +- tests/libslic3r/test_sketchprofile.cpp | 10 ++-- tests/libslic3r/test_voronoi.cpp | 17 ++----- .../test_plugin_capability_config.cpp | 22 ++++---- 9 files changed, 79 insertions(+), 68 deletions(-) diff --git a/tests/fff_print/test_cooling.cpp b/tests/fff_print/test_cooling.cpp index be8c29d2e3..d5b83ba570 100644 --- a/tests/fff_print/test_cooling.cpp +++ b/tests/fff_print/test_cooling.cpp @@ -1,7 +1,7 @@ #include #include -#include "catch2/catch_message.hpp" +#include #include "test_helpers.hpp" #include @@ -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) { diff --git a/tests/fff_print/test_print.cpp b/tests/fff_print/test_print.cpp index 55f82894c1..8ab07bbaaa 100644 --- a/tests/fff_print/test_print.cpp +++ b/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(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)); } } } diff --git a/tests/fff_print/test_trianglemesh.cpp b/tests/fff_print/test_trianglemesh.cpp index e43185d2f6..f96dbfa9a8 100644 --- a/tests/fff_print/test_trianglemesh.cpp +++ b/tests/fff_print/test_trianglemesh.cpp @@ -2,7 +2,7 @@ #include #include -#include "catch2/matchers/catch_matchers.hpp" +#include #include "libslic3r/TriangleMesh.hpp" #include "libslic3r/TriangleMeshSlicer.hpp" #include "libslic3r/Point.hpp" diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index 28133550b8..14d192c6fe 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -44,6 +44,7 @@ #include #include +#include #include #include #include @@ -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 /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 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) --- diff --git a/tests/libslic3r/test_preset_bundle_loading.cpp b/tests/libslic3r/test_preset_bundle_loading.cpp index 1a05cc1d3f..97e4a00503 100644 --- a/tests/libslic3r/test_preset_bundle_loading.cpp +++ b/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{ "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("compatible_printers")->values == std::vector{ "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{ "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{ "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("compatible_printers")->values == + std::vector{ "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{ "New Printer" }); diff --git a/tests/libslic3r/test_sketchimport.cpp b/tests/libslic3r/test_sketchimport.cpp index cdbf0b8296..77c8c6e979 100644 --- a/tests/libslic3r/test_sketchimport.cpp +++ b/tests/libslic3r/test_sketchimport.cpp @@ -7,6 +7,7 @@ #include "libslic3r/CAD/SketchImport.hpp" #include "test_utils.hpp" // ScopedTemporaryFile +#include #include #include #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 /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. diff --git a/tests/libslic3r/test_sketchprofile.cpp b/tests/libslic3r/test_sketchprofile.cpp index 263a5fc73f..de14b4b263 100644 --- a/tests/libslic3r/test_sketchprofile.cpp +++ b/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]") diff --git a/tests/libslic3r/test_voronoi.cpp b/tests/libslic3r/test_voronoi.cpp index 46570440e0..940b7f0097 100644 --- a/tests/libslic3r/test_voronoi.cpp +++ b/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; // 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); diff --git a/tests/slic3rutils/test_plugin_capability_config.cpp b/tests/slic3rutils/test_plugin_capability_config.cpp index b86eda8d22..4235d2d22e 100644 --- a/tests/slic3rutils/test_plugin_capability_config.cpp +++ b/tests/slic3rutils/test_plugin_capability_config.cpp @@ -8,7 +8,7 @@ #include #include -#include "catch2/catch_message.hpp" +#include #include "plugin_test_utils.hpp" #include #include @@ -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"}}));