Propose applicability test cases for non-Entity facets (#181) - #453
Propose applicability test cases for non-Entity facets (#181)#453BIMvoice wants to merge 2 commits into
Conversation
Every applicability block in the shipped test suite used only the Entity facet. Add three pass/fail pairs proving Classification, Property (AND-combined with Entity) and Attribute facets also work in applicability, per berlotti's confirmation on buildingSMART#181 and the worked examples in specifications.md. Hand authored since the CreateTestCases .NET build does not run in this environment. Validated against Schema/ids.xsd (xmlschema) and functionally against ifctester on both current v0.8.0 IfcOpenShell and the local ifctester source. AI-assisted contribution, human reviewed before submission.
|
Copying over from another PR #458 where you regenerated the TestCases:
So it should be relatively simple to update the SchemaProject Tooling to handle all the facets on Applicability. The
I would update this - no point having an invalid test in the PR. |
Implement Make<Facet> for Attribute, Classification, Material, PartOf and Property in the Applicability IDS types, and wire them into PopulateApplicability so the generator no longer silently drops these facets. Cardinality is dropped since it is a requirements-only concept per the schema. Also fix Error 305 by lowercasing TRUE to true in the fire rating fixtures and their scripts.md source, per XSD boolean rules.
andyward
left a comment
There was a problem hiding this comment.
Can you remove this section from all the new Make<Facet> methods:
if (parts.Count > 0 && parts[0] is ConditionalCardinality)
{
// cardinality is a requirements-only concept, not available on applicability facets
parts.RemoveAt(0);
}
|
Both done, pushed as The five I left cardinality out. In Proved it works rather than assuming: reverted
One thing I did not touch: |
Great
That's because there can be only one Entity facet per applicability. It doesn't make sense that multiple be supplied, whereas other facets can be combined. |
This issue is assigned to @CBenghi, so this is offered as a starting point to take, change or discard, not a claim on the work.
The measured gap
Parsing all 318
.idsfiles inTestCases/: every single<applicability>block uses only the Entity facet. None uses Classification, Attribute, Property, Material or PartOf in applicability, and none carriesminOccursormaxOccurson an applicability facet.Cross-checked against
Schema/ids.xsd:applicabilityType's facets use the bare types with nocardinalityattribute, onlyrequirementsType's extensions add it. So per-facet Prohibited or Optional at the applicability level genuinely does not exist in 1.0; only whole-specification cardinality does, and that is already covered.What these cases implement
Driven by the concrete asks in the thread rather than by the label. @berlotti established the semantics in the first reply, "there is symmetry in applicability and requirements" and "union. So multiple facets should be interpreted as AND". @CBenghi later enumerated the missing cases.
Three pass and fail pairs, each pair sharing the same specification so a pass alone cannot be mistaken for not-checked-at-all:
specifications.mdverbatimHow these were built
Correction: I originally wrote that
CreateTestCasescould not be run. That was wrong. The build needs the .NET 8 runtime specifically, because Nuke usesBinaryFormatter, which .NET 9 removed. With that installed it runs in about a second. These.idsfiles are still hand-authored; I am happy to regenerate them through the official build on request. Validation, unchanged: all six are XSD-valid againstSchema/ids.xsd(checked with a deliberately broken control file to confirm the validator actually rejects invalid input), and all six give the expected pass or fail verdict against IfcOpenShell's IfcTester.Separate observation, not addressed here
Sweeping the existing suite with current IfcTester, 18 shipped
pass-cases return FAIL, sharing one cause: theAttributefacet does not checkcardinality == "optional"before failing on a null value with a value constraint. That is a defect on our side rather than in these cases, and we have a fix in review at IfcOpenShell#9205 that adds exactly that check. Mentioned only so the number is not mistaken for a problem with this suite.@CBenghi @berlotti
This PR was created with the assistance of an AI coding tool.