[GeoMechanicsApplication] Make sure all 'integration scheme related' functions throw an exception #12642
Conversation
…on for our line interface geometry
| KRATOS_TEST_CASE_IN_SUITE(InterfaceGeometry_Throws_WhenCallingFunctionsRelatedToIntegrationPoints, | ||
| KratosGeoMechanicsFastSuiteWithoutKernel) | ||
| { | ||
| auto geometry = CreateThreePlusThreeNodedLineInterfaceGeometry(); | ||
| Geometry<Node>::JacobiansType dummy_jacobian; | ||
| Geometry<Node>::ShapeFunctionsGradientsType dummy_shape_functions_gradients; | ||
| Vector dummy_vector; | ||
| Matrix dummy_matrix; | ||
| IndexType dummy_index = 0; | ||
| GeometryData::IntegrationMethod dummy_integration_method = GeometryData::IntegrationMethod::GI_GAUSS_1; | ||
| Geometry<Node>::IntegrationPointsArrayType dummy_integration_points; | ||
| IntegrationInfo dummy_integration_info(dummy_index, dummy_integration_method); | ||
| Geometry<Node>::GeometriesArrayType dummy_geometries; | ||
| std::vector<Node::CoordinatesArrayType> dummy_coordinates; | ||
| const std::string message = | ||
| "This Geometry type does not support functionality related to integration schemes.\n"; | ||
|
|
||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.Normal(dummy_index), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.Normal(dummy_index, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.UnitNormal(dummy_index), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.UnitNormal(dummy_index, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.Jacobian(dummy_jacobian, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.Jacobian(dummy_jacobian, dummy_integration_method, dummy_matrix), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.Jacobian(dummy_matrix, dummy_index, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.Jacobian(dummy_matrix, dummy_index, dummy_integration_method, dummy_matrix), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.DeterminantOfJacobian(dummy_vector, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.DeterminantOfJacobian(dummy_index, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.InverseOfJacobian(dummy_jacobian, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.InverseOfJacobian(dummy_matrix, dummy_index, dummy_integration_method), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.ShapeFunctionsIntegrationPointsGradients( | ||
| dummy_shape_functions_gradients, dummy_integration_method), | ||
| message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.ShapeFunctionsIntegrationPointsGradients( | ||
| dummy_shape_functions_gradients, dummy_vector, dummy_integration_method), | ||
| message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.ShapeFunctionsIntegrationPointsGradients( | ||
| dummy_shape_functions_gradients, dummy_vector, dummy_integration_method, dummy_matrix), | ||
| message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.CreateIntegrationPoints(dummy_integration_points, dummy_integration_info), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN(geometry.GetDefaultIntegrationInfo(), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.CreateQuadraturePointGeometries(dummy_geometries, dummy_index, | ||
| dummy_integration_points, dummy_integration_info), | ||
| message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.CreateQuadraturePointGeometries(dummy_geometries, dummy_index, dummy_integration_info), message) | ||
| KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
| geometry.GlobalSpaceDerivatives(dummy_coordinates, dummy_index, dummy_index), message) | ||
| } |
There was a problem hiding this comment.
I was thinking a bit more about this test and I'm actually doubting if we should keep it. It does not test any logic, it just repeats the list of functions that we think should throw. I'm not sure if it's worth the time it takes to set up these kinds of tests. On the other hand, it will flag if someone implements one of the functions at some point, so I'm in two minds.
In general it's good to test for exceptions, but in my opinion that's more in the sense of functions throwing when the input/state is unexpected.
If we would decide these kinds of tests are not worth the time of setting them up, we should remove this particular one in my opinion, since it would encourage making more of them.
@avdg81 I'm curious to hear your thoughts on this
There was a problem hiding this comment.
If we start from the idea that unit tests can also be used as a form of "living" documentation (i.e. it is always up-to-date since it executes the code under test) then I do see the added value of this test case. Otherwise, the unit tests leave this part of the public API undocumented, forcing the developer to look into the implementation to see what she can expect. So I think I'd still prefer to have the new unit test in place. @WPK4FEM What do you think?
There was a problem hiding this comment.
I would vote for having these tests. Yes they are repetitive of the code as it is now, but they will flag on behavioral changes.
On the setup of the dummy geometry for the unit tests I have a question, but that I will raise in the review.
There was a problem hiding this comment.
Indeed these are good points, then I agree they bring enough value for the effort, let's leave the test in 👍
There was a problem hiding this comment.
Dear Richard,
Thank you for the consistent work. I have compared against the list given by Anne in the issue and against geometry.h. Your implementation consistently overrides the functionality to block.
My remarks are not blocking, only something I'd like to understand better myself.
applications/GeoMechanicsApplication/custom_geometries/line_interface_geometry.h
Show resolved
Hide resolved
...ations/GeoMechanicsApplication/tests/cpp_tests/custom_geometries/test_interface_geometry.cpp
Show resolved
Hide resolved
rfaasse
deleted the
geo/12641-disable-api-related-to-integration-schemes-for-the-new-line-interface-geometry
branch
📝 Description
Make sure the integration scheme related functions throw, such that when they are called it is clear that this geometry won't have that functionality.