AdSecGH-63 Fix unflatten Test #98
Conversation
DominikaLos
changed the title
|
@DominikaLos there is a failure on the test due to the pre-commit module. Can you make sure you setup this locally? I have added instruction how to do that on the README. Once you do this you will be able to do |
|
|
||
| TestSection(flattened, actualSection); | ||
| } | ||
|
|
||
| private void TestSection(ISection expected, ISection actual) { | ||
| private static void TestSection(ISection expected, ISection actual, bool unflattened = false) { |
There was a problem hiding this comment.
can we put this comparison at the core GSA.GH project withi this signature
public class SectionHelper
{
public static bool Equal(ISection sectionA, ISection sectionB, bool unflattened){...}
}then this method can simply call the previous one
{
Assert.True(SectionHelper.Equal(expected, actual, unflattened))
}
| Assert.Equal(((ISingleBars)expected.ReinforcementGroups[0]).Positions[0].Z.Value, ((ISingleBars)actual.ReinforcementGroups[0]).Positions[0].Z.Value, 4); | ||
| Assert.Equal(((ISingleBars)expected.ReinforcementGroups[0]).BarBundle.CountPerBundle, ((ISingleBars)actual.ReinforcementGroups[0]).BarBundle.CountPerBundle); | ||
| Assert.Equal(((ISingleBars)expected.ReinforcementGroups[0]).BarBundle.Diameter, ((ISingleBars)actual.ReinforcementGroups[0]).BarBundle.Diameter); | ||
| if (!unflattened) { |
There was a problem hiding this comment.
@SandeepArup is there a method for testing if a Section if unflattened or flatten? I don't like that we dictate if that is the case. There must be an internal flag we can reuse?
There was a problem hiding this comment.
@RaviKumarGubbala @vilayatk is there any flag which tell section is unflattened or flatten?
There was a problem hiding this comment.
Such kind of check shouldn't be there. Is it added becuase test are failing? I think the test fails becuase CountPerBundle cant be checked before after flatten. If there are three bars per bundle, it might be geting converted to three single bars. After confirming the behaviour we could update/delete the check related CountPerBundle. @SandeepArup
There was a problem hiding this comment.
I agree @RaviKumarGubbala. However, "It might be" OR always converting bar in bundle to single bars?
These tests are json sterilization and deserialization test i.e. b/w flatten and original section and vice-versa and have only one bar in bundle. So, test should not fail in either case. @DominikaLos lets discuss this tomorrow.
There was a problem hiding this comment.
@psarras I had some discussion with @RaviKumarGubbala and have feeling that after flattening section, profile will get changed(although section geometry will remain same) and so center of gravity and rebar position will be changed relative to new Origin/CG position and there is no guarantee that we can match the position with original section.
And second things, why these tests? Should we write test for third party library? If consuming Revit API or Grasshopper API, should we write test for their functionalities? If we are finding some issue in AdSec API then It should be reported to AdSec team for resolution.
So, in my opinion, these test should not be here.
There was a problem hiding this comment.
@SandeepArup Hey! I agree we shouldn't be testing GSA.API here. I will need to look into more detail to understand if we simply remove them, however, I have the feeling they are also testing some secondary functionality, of which we need a test. So perhaps we morph this problem to something that helps us.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
=====================================
Coverage 23.1% 23.1%
=====================================
Files 77 77
Lines 6197 6197
Branches 811 811
=====================================
Hits 1437 1437
Misses 4729 4729
Partials 31 31 |
No description provided.