Add IFC2X3 occurrence/type mapping test case (#116) - #458
Conversation
atomczak on buildingSMART#116 (2024-08-26): "documentation already in place, missing one pass/fail test case to verify it." PR buildingSMART#301 merged the ifc2x3-occurrence-type-mapping-table.md but no fixture in TestCases exercises it, and no PR after buildingSMART#301 references buildingSMART#116. Adds a pass/fail pair under Documentation/ImplementersDocumentation/ TestCases/entity using the IfcAirTerminal example the issue itself raises: applicability matches the real IFC2X3 occurrence class IfcFlowTerminal, and the requirement checks the entity facet name IFCAIRTERMINAL, which only resolves via the mapping table's IfcAirTerminalType. The fail fixture reuses the same specification against an IfcFlowTerminal typed by IfcElectricApplianceType instead, so a false pass could not be mistaken for a correctly-checked type mismatch. A third control fixture (untyped IfcFlowTerminal) also fails, confirming applicability alone does not vacuously pass. Hand authored to match the format produced by CreateTestCases, since the .NET build could not be run locally. Validated against Schema/ids.xsd (plus a deliberately malformed control) and against IfcOpenShell's IfcTester (src/ifctester, the implementation of IfcOpenShell#7212), which returned the expected pass/fail/fail verdicts for the pass, fail and control fixtures. AI-assisted with Claude Code.
andyward
left a comment
There was a problem hiding this comment.
If we're going to add these cases I'd suggest making more practical. My concern is they may miss the more likely real world edge cases in this IFC 2x3 compatibility feature
This needs resolution on your end as the 'hand authored' is subtly different to the generated versions (just in whitespace), which creates confusion when we next regenerate. The tooling requires .NET8 runtime to be installed. Perhaps you're missing that?
That's because of PR #433 - I'd guess |
|
Sidebar discussion for Petru @BIMvoice - appreciate the good intentions of these PRs and comments, (ably assisted by AI agents), but I think the IDS team need a discussion about the impact on volunteer maintainers who need to review and approve these submissions - and exercise that can't realistically be handed over to agents. |
andyward on buildingSMART#458 (2026-08-06): applicability should be expressed on IfcAirTerminal, not IfcFlowTerminal, and should cover a realistic naming requirement, a minOccurs=1 "must exist" requirement, and PredefinedType including USERDEFINED, since 2X3 hides that complexity on the type object. Measured first: IfcTester's Entity facet already resolves IFCAIRTERMINAL through the type mapping table on BOTH the applicability path (Entity.filter, src/ifctester/ifctester/facet.py:208-218, falls back to by_type(f"{name}Type") plus get_types when by_type(name) raises) and the requirements path (Entity.__call__, facet.py:236-243, falls back to get_type(inst) plus a TYPE-suffix check). Confirmed empirically against a minimal IFC2X3 IfcFlowTerminal typed by IfcAirTerminalType: applicability selected the element and the requirement matched. No IfcTester defect; no code change needed. Replaces the single pass/fail pair with four pairs, each applicability expressed as IFCAIRTERMINAL: - Name pattern requirement (AIR-XXX), matching his "All AirTerminals should be named like AIR-XXX" scenario. - minOccurs=1 "there must be AirTerminals in the model", applicability alone, no requirements section, replacing the fail case he called unnatural. - PredefinedType enumeration value on IfcAirTerminalType. - PredefinedType USERDEFINED, with ElementType carrying the custom label per util.element.get_predefined_type's IFC2X3 fallback. Each fail fixture reuses its pass fixture's specification against a model that fails for the documented reason (name mismatch, zero applicable entities, or predefined type mismatch), verified with IfcTester so no fixture passes vacuously. All eight also validate against Schema/ids.xsd; a deliberately malformed control confirmed the validator discriminates. Hand authored to match the format produced by CreateTestCases, since the .NET build could not be run locally. AI-assisted with Claude Code.
|
Thanks Andy, this is a better shape than what I had. Pushed just now, all four points in.
On your point about selection and verification differing: I checked before touching the fixtures, and IfcTester resolves the mapping on both paths already, so this is fixture work only, no implementation change. Each fail fixture reuses its pass fixture's specification and fails for the documented reason. The three requirement-based ones match one applicable entity and fail on the requirement, so none of them passes or fails vacuously. On your sidebar, you are right and I would rather you had not had to say it. I have stopped. 9 PRs in three days against a volunteer team is not a rate anyone can absorb, and review is exactly the part that cannot be handed to an agent. One of my comments was also simply wrong and you spent your own time correcting it across four threads, which is the cost you are describing. I have 8 open PRs. Tell me which you want and I will close the rest myself, no explanation needed. Closing all 8 is a fine answer. I will not open anything new here until the team has had that discussion and told me what it wants. If you do want more later, I would suggest one at a time, only where a maintainer has asked, and nothing new until the previous one is merged or closed, so the queue is capped at one regardless of how fast I can produce. Happy to be the test case if the team writes an AI contribution policy, including if the answer is no. |
andyward
left a comment
There was a problem hiding this comment.
Did you get the code generation from Script.md working?
Ran the official RepositoryAutomation CreateTestCases target rather than hand authoring. Output is byte identical to the hand written fixtures apart from the trailing newline, which the generator omits.
|
Good question, and the honest answer is that my earlier claim was wrong. It does work. I had said the build could not be run here. What actually happens is that it builds fine and then Nuke throws So these are generated now, not hand crafted. Pushed as Worth reporting what the regeneration showed: the generated output was byte identical to the hand written files apart from the trailing newline, which the generator omits. No other file in the suite changed, so there is no drift elsewhere. I have the same wrong claim in the bodies of #451, #452, #453 and #457, and I am correcting those now. If any of them survive the clear-out we discussed, say the word and I will regenerate them the same way. It takes a second now that the runtime is sorted. |
andyward
left a comment
There was a problem hiding this comment.
Great this last update fixes any future noise when regenerating so please regen any other PRs where you hand crafted IDS test cases.
The github action build is failing on this PR. I think you also need to run build.sh AuditDocTestCases and ensure a clean run (zero error_code). It's likely some generated IDS has a content issue. (This could actually be the fact that the IDS audit tool needs updating - it only recently was updated to support this specific occurrence/type mapping issue- buildingSMART/IDS-Audit-tool@a2ec1ac)
|
Regenerated as asked. #457 and #451 are done and both now pass the audit; #457 was trailing newline only, #451 also swapped Two things worth reporting rather than quietly working around. #458 cannot reach zero error_code with any released auditor. On 1.0.86 it is the
In IFC2X3 the predefined type lives on #453 I did not regenerate, because regenerating it destroys the PR. Separately, and my fault rather than the tooling's, #453's current fixtures fail with |
|
There's a fix in the queue for the IDS audit tool. Once that's approved we can incorporate the latest tool which supports validating predefined types on 2x3 mapped IFC4 entities |
Review was completed by regeneration of IDS files and update of the tooling
Delivers the one item @atomczak flagged as outstanding on #116.
His wrap-up there, 2024-08-26: "documentation already in place, missing one pass/fail test case to verify it." PR #301 merged
Documentation/ImplementersDocumentation/ifc2x3-occurrence-type-mapping-table.md, but nothing inTestCasesexercises it, and no PR since references #116.What this adds
A pass and fail pair under
TestCases/entity, using theIfcAirTerminalexample the issue itself raises.Applicability matches the real IFC2X3 occurrence class
IFCFLOWTERMINAL. The requirement checks the entity facet nameIFCAIRTERMINAL, which in IFC2X3 only resolves through the mapping table'sIfcAirTerminalType.The fail fixture reuses the same specification against an
IfcFlowTerminaltyped byIfcElectricApplianceTypeinstead. Same applicability, different type, opposite verdict, so a pass alone cannot be mistaken for not-checked-at-all.Validation
The
.idsfiles are generated by the repository's ownCreateTestCasestarget. Note for anyone hitting the same wall: the build needs the .NET 8 runtime specifically, because Nuke usesBinaryFormatter, which .NET 9 removed. They were also verified two ways here:.idsfiles validate againstSchema/ids.xsd. A deliberately malformed control, with<entity>renamed to a bogus facet, correctly fails, confirming the validator discriminates rather than passing anything.IfcFlowTerminalalso fails, which confirms the applicability facet genuinely selects the instance in all three cases rather than matching nothing.That third control is deliberate. Two fixtures already in this suite pass vacuously without testing what their names say, one of which we reported as #456, so proving applicability actually matches seemed worth doing explicitly.
Happy to have this regenerated through the official build if that is preferred.
Note on branches
The mapping table differs between branches.
developmentcarries the corrected values,IfcSpaceHeatertoIfcEnergyConversionDeviceandIfcVibrationIsolatortoIfcEquipmentElement, whilever/1.1.xstill has the earlier ones. These fixtures targetdevelopmentand useIfcAirTerminal, which is unaffected by that difference.This PR was created with the assistance of an AI coding tool.