search

BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 12 forks source link

Structure_Engine: Fix stack overflow exception for Area for FEMesh #3330

Closed IsakNaslundBh closed 2 months ago

IsakNaslundBh commented 3 months ago

Issues addressed by this PR

Closes #3323

Found by investigating issue with unit tests. Problem was a real stack overflow exception in the area method for the FEMesh.

All structural dataset re-serialised to account for the change from StartNode to Start etc on the bars.

Test files

Unit-tests should now run.

Changelog

Additional comments

IsakNaslundBh commented 3 months ago

@BHoMBot check compliance @BHoMBot check unit-tests

bhombot-ci[bot] commented 3 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` - check `unit-tests`
IsakNaslundBh commented 3 months ago

@BHoMBot check unit-tests

bhombot-ci[bot] commented 3 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `unit-tests`
FraserGreenroyd commented 3 months ago

@BHoMBot check unit-tests

bhombot-ci[bot] commented 3 months ago
@FraserGreenroyd to confirm, the following actions are now queued: - check `unit-tests` There are 2 requests in the queue ahead of you.
bhombot-ci[bot] commented 2 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `unit-tests`
bhombot-ci[bot] commented 2 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `unit-tests`
IsakNaslundBh commented 2 months ago

Have tried to get all structures UTs working as intended now.

The only one still failing locally is the Cellular section, and that is a serialisation issue rather than a UT issue.

It is coming from the change to how the name is handled in https://github.com/BHoM/BHoM_Engine/pull/3144 . Where the obejct returned has an empty name, but when serialised and de-serialised it comes back with a null name.

Might be worth raising an issue for, not sure. Could potentially set names on deserialization to emtpy strings rather than null, but that could cause similar issues the other way around. Other otption would be to roll back the change for the name made in the linked PR, or make it onyl skip name if null, rather than NullOrEMpty. Nontheless, out of scope for this particular PR.

IsakNaslundBh commented 2 months ago

The Bot seem to agree with me. Think this can be reviewed now. Only failing UT for structures is the one I mentioned above. The rest is also to be fixed, but not in this PR, but should be fixed by the corresponding code owners, which is @IsakNaslundBh , @alelom @peterjamesnugent @FraserGreenroyd (See current failing tests)

IsakNaslundBh commented 2 months ago

@BHoMBot check compliance @BHoMBot check required

bhombot-ci[bot] commented 2 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `core` - check `null-handling` - check `serialisation` - check `versioning` - check `installer`
bhombot-ci[bot] commented 2 months ago
The check `code-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.
bhombot-ci[bot] commented 2 months ago
The check `documentation-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.
bhombot-ci[bot] commented 2 months ago
FAO: @FraserGreenroyd @pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate. The check they wish to have dispensation on is unit-tests. If you are providing dispensation on this occasion, please reply with: > @BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. `24100093353`
IsakNaslundBh commented 2 months ago

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 24100093353

bhombot-ci[bot] commented 2 months ago
@IsakNaslundBh I have now provided a passing check on reference `24100093353` as requested.
IsakNaslundBh commented 2 months ago

Providing dispensation as of comment here: https://github.com/BHoM/BHoM_Engine/pull/3330#issuecomment-2069287165

IsakNaslundBh commented 2 months ago

@BHoMBot check ready-to-merge

bhombot-ci[bot] commented 2 months ago
@IsakNaslundBh to confirm, the following actions are now queued: - check `ready-to-merge`