VV: Update WriteDREAM3DFilter testing by mmarineBlueQuartz · Pull Request #1683 · BlueQuartzSoftware/simplnx

mmarineBlueQuartz · 2026-07-23T12:51:40Z

Add V&V files for WriteDREAM3DFilter
Expanded round-trip testing for additional DataObject types.
New tests for untested code paths.

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

1 Unit test to test output from the filter against known exemplar set of data
1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

No commented out code (rare exceptions to this is allowed..)
No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added example pipelines that use the filter
Classes and methods are properly documented

imikejackson

Full adversarial V&V review. I independently verified the report's claims against the tree at head 7d24b7d: all 17 Write-touching TEST_CASEs exist and pass locally (plus CI green on all 5 platforms), ProbeHdf5Dataset and the compression sub-suite are real, the error codes (-1/-2/-15), UUIDs, parametersVersion()==2, the FromSIMPLJson compression override, the 16 KiB doc threshold, and the cited PRs (#1606/#1544/#1253/#900) all check out. The 19-path enumeration is arithmetically consistent with the algorithm source, and the oracle is genuinely Class 1 and non-circular (all fixtures inline, no exemplar archive — so no provenance sidecar is correctly required).

Requesting changes for the items below. The blocking ones:

Dead writeXdmf GENERATE in the IO test, which also poisons the report's Path 17 citation.
The "every geometry and DataObject type" claim is false — RectGridGeom is never written by any test, and ScalarData/GridMontage are uncovered despite having IO classes.
Leftover dead code from an abandoned DynamicListArray round-trip (the type has no HDF5 IO at all — worth recording in the report instead of silently dropping).
NeighborList values are never compared after round-trip (only the tuple count).
The use_compression=false FromSIMPLJson override the report leans on is not asserted by the SIMPL BC test.

On the legacy A/B question: I agree no legacy comparison is meaningful here — the two writers target deliberately different on-disk contracts, and the only tool that can read both formats is simplnx's own reader, which would make a writer comparison circular. The Class 1 round-trip + HDF5 layout probing is the right oracle. See the inline suggestion on D1 to say so explicitly.

Remaining inline comments are nits (tags, CheckArraysInheritTupleDims, naming, fixture geometry validity, unused locals, cleanup).

Add V&V documentation

* Fixed DREAM3DIO checking the wrong result. * Added ScalarData and RectGridGeom testing to DREAM3DFileTest. * Improved NeighborList testing * Updated geometries to use arrays of the appropriate tuple components. * Check that the xdmf file exists.

* Added check that the use compression argument is disabled when importing from legacy SIMPL since it did not exist .

imikejackson

Re-review findings. Nine inline comments below; one additional issue lives outside this PR's diff so it goes here:

Xdmf node-geometry writers transpose geomName and hdf5FilePath — src/simplnx/Utilities/Parsing/DREAM3D/Dream3dIO.cpp lines 636, 648, 660: WriteXdmfNodeGeometry1D/2D/3D each forward to the next-lower writer with the last two arguments swapped (WriteXdmfNodeGeometry0D(out, nodeGeom1D, hdf5FilePath, geomName) against a callee signature of (out, geom, geomName, hdf5FilePath)). Both are std::string_view, so it compiles silently. Any .xdmf written for an Edge/Triangle/Quad/Tet/Hex geometry with a vertex attribute matrix emits Attribute DataItems that use the geometry name as the HDF5 file path, so ParaView/VisIt cannot resolve the node-centered arrays. This is pre-existing rather than introduced here, but the PR's V&V report claims Path 17 (xdmf) coverage, and the new CheckXdmfFile() oracle (existence-only) cannot catch it — the fix plus a content-level xdmf assertion belong with this V&V effort.

imikejackson · 2026-08-12T18:41:38Z

+  auto yBounds = std::make_shared<Float32DataStore>(yShape, componentBounds, 0.0f);
+  auto* yBoundsArray = Float32Array::Create(dataStructure, Constants::k_YBounds, yBounds, rectGridGeom->getId());
+  FillDataStore<float32>(*yBounds.get());
+  ShapeType zShape{Constants::k_TupleShape[1]};


Copy-paste bug: zShape is built from Constants::k_TupleShape[1] instead of [2], and all three RectGrid bounds arrays are sized dim instead of dim + 1. For k_TupleShape{3, 2, 1} the bounds must be x=4, y=3, z=2 — N cells need N+1 bounds, a contract RectGridGeom itself relies on (RectGridGeom.cpp reads xBnds->at(x + 1)). As written, getCoords/getSpacing on the read-back geometry throws out_of_range, and in the writeXdmf=true iteration the emitted Topology Dimensions="2 3 4" disagrees with the three DataItem dimensions (3/2/2), so the .xdmf is self-inconsistent and ParaView rejects the RectGrid grid.

imikejackson · 2026-08-12T18:41:39Z

+  REQUIRE(triangleGeom != nullptr);
+
+  // 3D Geometries
+  auto* hexGeom = Create3DGeom<HexahedralGeom>(dataStructure, Constants::k_HexGeom, *vertexArray, *edgesArray, *hexArray, *polyhedraArray);


The face and polyhedra lists are swapped in the Create3DGeom calls: the 8-component hexArray is installed as HexahedralGeom's FACE list while the 4-component "Polyhedra" array becomes the hex connectivity list — a hex cell needs 8 vertices (HexahedralGeom::k_NumVerts == 8), not 4. Same for TetrahedralGeom: a 4-component array becomes the face list where triangle faces need 3. CheckGeom3D asserts the same swapped IDs, so the round-trip is self-consistently wrong: the flagship content-fidelity test validates malformed hex/tet geometries, and a writer bug corrupting genuine 8-component hex connectivity would pass unnoticed.

imikejackson · 2026-08-12T18:41:39Z

+  UnitTest::LoadPlugins();
+  std::lock_guard<std::mutex> lock(m_DataMutex);
+
+  DataStructure dataStructure = CreateTestDataStructure();


Both SECTIONs here only call filter.preflight() and validate parameter guards (empty path, bad compression level) that never touch DataStructure contents, yet each SECTION re-entry builds the full CreateTestDataStructure() fixture (all 8 geometries, NeighborList, 10 scalars). The pre-existing Compression_Preflight_RejectsOutOfRangeLevel test does the same job with an empty DataStructure ds; — suggest the same here.

imikejackson · 2026-08-12T18:41:39Z

+  }
+}
+
+TEST_CASE("WriteDREAM3DFilter:Valid Parameters", "[ReadDREAM3DFilter][WriteDREAM3DFilter]")


None of the four new TEST_CASEs (Invalid Parameters, Valid Parameters, Pipeline / WriteXdmf combinations, Invalid File) calls UnitTest::CheckArraysInheritTupleDims(dataStructure); near the end, per the project testing convention. Valid Parameters is the one that matters most — it reads a full DataStructure back, which is exactly where a tuple-dimension inheritance regression in the reader/writer would go undetected. Other tests in this file do call it, so the omission reads as an inconsistency rather than a file-wide waiver.

imikejackson · 2026-08-12T18:41:39Z

+  REQUIRE(fs::exists(filepath));
+}
+
+TEST_CASE("WriteDREAM3D:Pipeline / WriteXdmf combinations", "[ReadDREAM3DFilter][WriteDREAM3DFilter]")


This TEST_CASE, Invalid File, Valid Parameters, and the rewritten DREAM3D File IO Test all write/read the single shared GetIODataPath() (newFile.dream3d) guarded only by the process-local m_DataMutex — but catch_discover_tests runs each TEST_CASE as an independent ctest process, so the mutex protects nothing across them. Under ctest -j N one process can HDF5-truncate the file while another is mid-read, producing intermittent "truncated file"/HDF5 open errors. The PR triples the contenders on this path; distinct per-TEST_CASE filenames would remove the hazard.

imikejackson · 2026-08-12T18:41:39Z

+
+  if(writeXdmf)
+  {
+    CheckXdmfFile();


This asserts the .xdmf sidecar EXISTS after a write that is required to fail — a failed write to fs::path() must not produce a sidecar, so the assertion is inverted. It only passes because a stale newFile.xdmf from the earlier Pipeline / WriteXdmf combinations TEST_CASE is still on disk at GetXdmfPath(). Run in isolation (test filter, --order rand, or a fresh output dir) and REQUIRE(fs::exists(...)) fails spuriously; run in order and it can never detect a regression where WriteFile starts emitting an .xdmf before validating the HDF5 path. Should be REQUIRE_FALSE(fs::exists(...)) against a path derived from the (empty) target, or dropped.

imikejackson · 2026-08-12T18:41:39Z

-    auto fileWriter = HDF5::FileIO::WriteFile(GetIODataPath());
-
-    auto writeResult = DREAM3D::WriteFile(fileWriter, fileData);
+    auto writeResult = DREAM3D::WriteFile(GetIODataPath(), CreateTestDataStructure(), CreateExportPipeline(), writeXdmf);


Two knock-on effects of switching this to the path-based DREAM3D::WriteFile overload: (1) the exported DREAM3D::WriteFile(HDF5::FileIO&, const FileData&) forwarder (Dream3dIO.cpp:2426) lost its only caller in the tree, so a future regression in it (e.g. swapping fileData.first/fileData.second) passes CI silently while breaking downstream callers; (2) CreateFileData() at line 692 is now dead code in the anonymous namespace — clang/gcc emit -Wunused-function, which fails warnings-as-errors builds. Either keep one call through the FileData overload or delete both the helper and (if truly unwanted) deprecate the overload.

imikejackson · 2026-08-12T18:41:39Z

+
+*Applied:* Every test constructs its expected answer directly, without ever running the filter to "produce" the expected value:
+
+- **Content fidelity:** each test builds a `DataStructure` in C++ (`CreateTestDataStructure()`, or an inline array/geometry), writes it, reads it back, and asserts the read-back content equals what was built — by construction, not by comparison to a previously-captured file. `CheckTestDataStructure()` walks every `DataObject` kind the filter must support (nested `DataGroup`s, `AttributeMatrix`, `NeighborList`, `StringArray`, and all seven geometry types: Vertex/Edge/Triangle/Quad/Tetrahedral/Hexahedral/Image) and asserts exact values against the hand-known fill pattern from `FillDataStore()`.


"all seven geometry types: Vertex/Edge/Triangle/Quad/Tetrahedral/Hexahedral/Image" — CheckTestDataStructure actually verifies eight: there is an explicit RectGrid block in the checker. RectGrid is missing from the formal record.

imikejackson · 2026-08-12T18:41:39Z

+
+| Test case | Status | Notes |
+|-----------|--------|-------|
+| `WriteDREAM3DFilter:Invalid Parameters` (§ Empty FilePath, § Bad Compression Level) | kept | Preflight-only, inline `DataStructure`. Covers Paths 1, 2. |


The inventory claims to be the complete list ("17 TEST_CASEs ... See Test inventory below for the full list") but omits DREAM3DFileTest: Geometry Nested In DataGroup Round Trip (tagged [WriteDREAM3DFilter], executes the filter directly across all 8 geometry types, added for issue #1642). An auditor using this table as the authoritative coverage record gets a wrong count and a missing test-to-code-path mapping.

* Fix WriteXdmfNodeGeometry1D/2D/3D forwarding geomName and hdf5FilePath in swapped order, which produced .xdmf node-attribute references that ParaView/VisIt could not resolve * Strengthen CheckXdmfFile to validate every heavy-data reference in the sidecar resolves to the written .dream3d file instead of only checking that the file exists, and add a vertex AttributeMatrix to the test fixture so node-centered attribute references are actually emitted * Fix swapped face/polyhedra connectivity lists for the Hexahedral and Tetrahedral test geometries (hex cells need the 8-component list as polyhedra and quad faces; tets need triangle faces) * Size RectGrid bounds arrays as N+1 for N cells and build zShape from k_TupleShape[2] instead of [1]; assert bounds sizes on read-back * Remove the inverted .xdmf existence assertion from the Invalid File test, which only passed via a stale sidecar from an earlier TEST_CASE * Give each file-writing TEST_CASE its own output filename so parallel ctest processes no longer race on newFile.dream3d * Add WriteDREAM3D:FileData Overload test so the exported WriteFile(FileIO&, FileData) forwarder and CreateFileData() stay covered * Use an empty DataStructure in the preflight-only Invalid Parameters test and add UnitTest::CheckArraysInheritTupleDims to the new tests * Correct the V&V report: 19 TEST_CASEs, eight geometry types including RectGrid, add the missing Geometry Nested In DataGroup Round Trip inventory row, and record the fixed xdmf bug under Bug flags Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>

mmarineBlueQuartz requested a review from nyoungbq July 23, 2026 12:51

nyoungbq approved these changes Jul 23, 2026

View reviewed changes

imikejackson requested changes Jul 23, 2026

View reviewed changes

mmarineBlueQuartz force-pushed the vv/WriteDream3dFilter branch from 7d24b7d to b7ba171 Compare July 24, 2026 13:46

mmarineBlueQuartz requested a review from imikejackson July 25, 2026 00:12

mmarineBlueQuartz added 4 commits August 7, 2026 09:40

Update WriteDREAM3DFilter testing

16e178a

Add V&V documentation

Clang format

aa87da3

PR requested changes

5925132

* Fixed DREAM3DIO checking the wrong result. * Added ScalarData and RectGridGeom testing to DREAM3DFileTest. * Improved NeighborList testing * Updated geometries to use arrays of the appropriate tuple components. * Check that the xdmf file exists.

PR changes for V&V docs

e36ddcf

* Added check that the use compression argument is disabled when importing from legacy SIMPL since it did not exist .

mmarineBlueQuartz force-pushed the vv/WriteDream3dFilter branch from 439d851 to e36ddcf Compare August 7, 2026 13:41

imikejackson reviewed Aug 12, 2026

View reviewed changes


		Applied: Every test constructs its expected answer directly, without ever running the filter to "produce" the expected value:

		- Content fidelity: each test builds a `DataStructure` in C++ (`CreateTestDataStructure()`, or an inline array/geometry), writes it, reads it back, and asserts the read-back content equals what was built — by construction, not by comparison to a previously-captured file. `CheckTestDataStructure()` walks every `DataObject` kind the filter must support (nested `DataGroup`s, `AttributeMatrix`, `NeighborList`, `StringArray`, and all seven geometry types: Vertex/Edge/Triangle/Quad/Tetrahedral/Hexahedral/Image) and asserts exact values against the hand-known fill pattern from `FillDataStore()`.

Uh oh!

Conversation

mmarineBlueQuartz commented Jul 23, 2026

Naming Conventions

Filter Checklist

Unit Testing

Code Cleanup

Uh oh!

imikejackson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imikejackson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants