VV: Update WriteDREAM3DFilter testing - #1683
Conversation
imikejackson
left a comment
There was a problem hiding this comment.
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
writeXdmfGENERATE 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=falseFromSIMPLJson 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.
7d24b7d to
b7ba171
Compare
* Added check that the use compression argument is disabled when importing from legacy SIMPL since it did not exist .
Naming Conventions
Naming of variables should descriptive where needed. Loop Control Variables can use
iif warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the filesimplnx/docs/Code_Style_Guide.mdfor a more in depth explanation.Filter Checklist
The help file
simplnx/docs/Porting_Filters.mdhas 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:
Code Cleanup