moose
moose copied to clipboard
Remove mesh_modifier tests and cleanup their descendants' tests
Closes #22267
Job Modules recover on 80b9953 : invalidated by @milljm
Job Coverage on 64fa053 wanted to post the following:
Framework coverage
Coverage did not change
Modules coverage
Coverage did not change
Full coverage reports
Reports
-
framework
-
chemical_reactions
-
combined
-
contact
-
electromagnetics
-
external_petsc_solver
-
fluid_properties
-
fsi
-
functional_expansion_tools
-
geochemistry
-
heat_conduction
-
level_set
-
misc
-
navier_stokes
-
peridynamics
-
phase_field
-
porous_flow
-
ray_tracing
-
rdg
-
reactor
-
richards
-
solid_properties
-
stochastic_tools
-
tensor_mechanics
-
thermal_hydraulics
-
xfem
This comment will be updated on new commits.
Job Documentation on 64fa053 wanted to post the following:
View the site here
This comment will be updated on new commits.
@socratesgorilla I already opened a PR for that. The issue is coverage went down, so there are some tests missing when you remove the mesh modifiers https://github.com/idaholab/moose/pull/20845
You're welcome to help on my PR, this PR is behind on the work that needs to be done though
All code coverage issues gone, ready for merge. I think there are strong reasons to prefer this over the other PR as well.
nice. I m happy to close the other one if we have all that was there. @lindsayad who was reviewing the other one or myself will look at this one.
All code coverage issues gone, ready for merge. I think there are strong reasons to prefer this over the other PR as well.
For a potential reviewer, can you outline the strong reasons?
All code coverage issues gone, ready for merge. I think there are strong reasons to prefer this over the other PR as well.
For a potential reviewer, can you outline the strong reasons?
The other PR keeps around a lot of tests that probably don't need to be there. For example, there are ~15 block_deletion_generator tests that all perform the same operation on slightly different meshes and the other PR keeps them, whereas here anything that does not seem to test important functionalities has simply been outright removed. If a test exists and removing it doesn't lose coverage, and the test isn't, for example, an exception test, or it doesn't test an otherwise important feature, then it has been removed. The result is this PR is much leaner than the previous one.
Coverage does not show the whole picture. These tests were likely designed to test a capability, not a line of code. Capabilities are much more important to test than lines of codes. What do these detail= fields say?
Unless you show that all those capabilities are still tested by other tests, those tests need to stay.
Coverage does not show the whole picture. These tests were likely designed to test a capability, not a line of code. Capabilities are much more important to test than lines of codes. What do these detail= fields say?
Unless you show that all those capabilities are still tested by other tests, those tests need to stay.
Give me a few minutes and I'll go through each test and compile a list on everything
Discussed a few of these with cody, if necessary we should all have a call/discussion about the merits of both sides. Regardless, here is the list of all the files I have moved/deleted and why:
-
add_all_sidesets, add_side_sets, add_side_sets_from_bounding_box, assign_subdomain_id, break_boundary, break_mesh_by_blocks, boundingbox_nodeset, parsed_sideset, parsed_subdomain, sidesets_between_subdomains, smooth_mesh, subdomain_bounding_box, transform: All these folders are duplicates of existing tests. All have been fully deleted.
-
add_extra_nodeset: Added exception test for bad coordinates to extra_nodeset_generator. Only meaningful difference between this folder's tests and those in extra_nodeset_generator is the mesh_modifier version uses a FileMesh for its input over a GeneratedMesh. Removed.
-
assign_element_subdomain_id:
- quad_with_subdomainid_test.i has functionality tested by "sidesets_between_subdomains_generator/sideset_between_vector_subdomains_generator.i", "userobjects/internal_side_user_object/internal_side_user_object_two_materials.i", and both tests in transfers/multiapp_transfer_transformation, all of which feature a QUAD4.
- quad_with_elementid_subdomainid_test.i has functionality tested in postprocessors/point_value/point_value.i
- For the remaining test, talked about it with cody, added in latest push.
-
block_deleter: Here are the details of the non-duplicate tests in this folder:
2: detail = "a 3D concave subdomain;" 3: detail = "a 2D interior subdomain;" 4: detail = "a 3D interior subdomain;" 5: detail = "a 2D non-concave subdomain;" 6: detail = "a 3D non-concave subdomain;" 7: detail = "a 2D removal of a union of disjoint pieces;" 8: detail = "a 2D removal of a subdomain containing a nodeset;" 9: detail = "a 2D removal of a subdomain that eliminates sideset;" 10: detail = "a 2D removal of a subdomain containing a sideset;" 12: detail = "a 2D concave subdomain with a cut across elements."
The problem with all of these tests is why they might seem like useful tests, and perhaps are, they aren't testing anything meaningful about the block deletion. ElementDeletionGeneratorBase does not have any logic for doing anything specific for these cases, it merely deletes the elems with no granularity. What is actually being tested here is the contract() and prepare_for_use() functions in libMesh, which is where those tests should be located if they are necessary. They aren't testing any particular functionality of the block deletion and I feel removing them is valid. The only one that has been kept was the one testing the deprecated param and parallel aspects for code coverage.
-
image_subdomain:
- image_2d.i has functionality tested in preconditioners/fsp/fsp_test_image.i
- image_3d.i does not test anything else meaningful.
-
lower_d_block:
- The first_order test is a duplicate of ids.i in lower_d_block_generator
- The second_order test can be found in many of the mortar tests, but just for an example, mortar/aux-gap/mismatch.i captures the same functionality.
-
mesh_extruder: All duplicates except tri, which was moved.
-
modifier_depend_order
- depend_with_prepare.i was testing a removed param and has no other meaningful tests
- modifier_depend_order.i seems to absolutely be a weirdly converted mesh_modifier test and TransformGenerators are tested plenty in mesh_generators/transform_generator and in things like cyclic.i. Removed.
-
sidesets_around_subdomain: Duplicates, but parallel coverage was lost and replaced.