moose icon indicating copy to clipboard operation
moose copied to clipboard

Remove mesh_modifier tests and cleanup their descendants' tests

Open socratesgorilla opened this issue 2 years ago • 11 comments

Closes #22267

socratesgorilla avatar Sep 29 '22 19:09 socratesgorilla

Job Modules recover on 80b9953 : invalidated by @milljm

moosebuild avatar Sep 29 '22 19:09 moosebuild

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

This comment will be updated on new commits.

moosebuild avatar Sep 29 '22 22:09 moosebuild

Job Documentation on 64fa053 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild avatar Sep 30 '22 18:09 moosebuild

@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

GiudGiud avatar Sep 30 '22 21:09 GiudGiud

All code coverage issues gone, ready for merge. I think there are strong reasons to prefer this over the other PR as well.

socratesgorilla avatar Oct 04 '22 15:10 socratesgorilla

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.

GiudGiud avatar Oct 04 '22 16:10 GiudGiud

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?

lindsayad avatar Oct 04 '22 16:10 lindsayad

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.

socratesgorilla avatar Oct 04 '22 16:10 socratesgorilla

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.

GiudGiud avatar Oct 04 '22 17:10 GiudGiud

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

socratesgorilla avatar Oct 04 '22 17:10 socratesgorilla

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:

    1. 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.
    2. quad_with_elementid_subdomainid_test.i has functionality tested in postprocessors/point_value/point_value.i
    3. 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:

    1. image_2d.i has functionality tested in preconditioners/fsp/fsp_test_image.i
    2. image_3d.i does not test anything else meaningful.
  • lower_d_block:

    1. The first_order test is a duplicate of ids.i in lower_d_block_generator
    2. 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

    1. depend_with_prepare.i was testing a removed param and has no other meaningful tests
    2. 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.

socratesgorilla avatar Oct 04 '22 19:10 socratesgorilla