Consolidate and clean mesh-modifer/generator tests

[GiudGiud]

refs #13814
Here's what happened to each test folder. Duplicates are exact save for object names and at times comments in input files.

• add_all_side_sets : duplicates of add_all_side_sets_generators, deleted
• break_boundary : duplicates of break_boundary_on_subdomain, deleted
• parsed_subdomain : duplicate of parsed_subdomain_generator, deleted
• add_extra_nodeset: similar test intent but different tests, consolidated with extra_nodeset_generator. Turned from diffusion problem to mesh-only exodus
• break_mesh_by_blocks
  Files renamed with this convention:
  TestBreakMesh_3D_Polycristal -> break_mesh_3D_polycrystal.i
  TestBreakMesh_3D_3Block_Auto.i -> break_mesh_3D_auto.i
  More tests in mesh generator folder
• sidesets_around_subdomain
  All 4 inputs for all 4 tests are in the new MG folder
  convention roughly for renaming: around_multi_created_subdomain.i <- sidesets_around_subdomain/around_multi_created_subdomain.i
• add_side_sets: 2 tests were duplicates with sidesets_from_normals_generator, deleted. Last one duplicated in sidesets_from_points_generator, deleted
• sidesets_between_subdomains
  all input files and tests are in the new MG folder. same names
• add_side_sets_from_bounding_box
  tests are duplicates of tests in meshgenerators/sidesets_bounding_box_generator
• lower_d_block
  The new tests are testing name/id input of sidesets. The old test were about first/second order meshes. The mesh modifier tests were moved to the mesh generators
• smooth_mesh
  duplicate of smooth_mesh_generator
  same mesh is used, the only difference is 2 less smoothing runs (out of 5) in the new test, and the old test was diffusion. The new test is mesh-only. Testing is equivalent overall
• assign_element_subdomain_id
  moved to MG folder
• mesh_extruder
  all duplicates except the tri test which was only in mesh modifier folder and got moved over to the mesh generator folder
• subdomain_bounding_box
  duplicate of subdomain_bounding_box_generator Tests
  the only difference is the 2-level refine in the mesh modifier test. This doesnt really affect anything, it just made the original test more expensive.
• assign_subdomain_id
  duplicate of subdomain_id_generator
  original test was a diffusion solve, replaced by mesh-only
• mesh_side_set
  Total duplicate of mesh_side_set_generator test
• transform
  total duplicate of transform_mesh_generator
• block_deleter
  The mesh modifier tests covered 12 different cases. The new mesh generator tests covered the 1st one then a 13th and 14th additional case. While input
  files (and gold files) were checked in for the 12 cases in the mesh_generator folder, the test specs were not written. The mesh modifier folder was deleted
  and the mesh generator test specs are now written for those 11 cases.
• modifier_depend_order
  The depend_with_force_prepare test is testing a parameter that no longer exists (force_prepare), so it's not really doing much. It's removed now
  The other test tests ordering of mesh generators. It has no direct equivalent in the new tests but it does overall gets tested a lot.
  I moved the folder over to meshgenerators/order_of_generation
• boundingbox_nodeset
  duplicate of boundingbox_nodeset_generator folder. The mesh modifier tests were diffusion solves, mesh generator tests are --mesh-only. Same testing power
• parsed_sideset
  test is part of the parsed_generate_sideset suite (which includes more)
• image_subdomain
  The mesh modifier tests were testing stacks of images and 3D images, while the tests in image_mesh_generator/ only had a single image test. The mesh_modifier folder tests were moved to the mesh generator corresponding folder

[moosebuild]

Job Documentation on 96a28d0 wanted to post the following:

View the site here

This comment will be updated on new commits.

[moosebuild]

Job Coverage on 96a28d0 wanted to post the following:

Framework coverage

	d71f8b	#20845 96a28d
	Total	Total	+/-	New
Rate	84.30%	84.31%	+0.00%	-
Hits	80164	80168	+4	0
Misses	14927	14923	-4	0

Diff coverage report

Full coverage report

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.

[lindsayad]

should we expect the coverage change?

[lindsayad]

(not that it is much)

[GiudGiud]

no we shouldnt. Seems the exception checking was more thorough for mesh modifiers than for mesh generators

[GiudGiud]

I ll go grab these tests

[GiudGiud]

let s see if that fixed it

[GiudGiud]

ok there's actual differences in the tests, despite the matching test & file names and descriptions. I ll try to find out what they are. I m thinking 3 tests are involved based on the diff

[lindsayad]

Huh? It looks like all tests are passing to me

[lindsayad]

well other than the doc

[GiudGiud]

I m talking about the diff in coverage.

[lindsayad]

ah, got ya

[lindsayad]

So what are we doing about this vs. #22267? It seems like you think this one is superior ... and I definitely agree that feature coverage is more important than line coverage

[GiudGiud]

I'm fixing this one to get the coverage right. The missing lines might tell us something about differences in the tests.

The other one has modified tests, a muddled commit history, deleted tests we do not want to delete, checks in the repository a duplicate of a picture of a kitten, pick your reason but imo it's not mergeable as is.

[GiudGiud]

so the final word on the missing lines is that you needed to run at least 3 processes in distributed mode (on several of the tests in that folder) to hit the missing lines.

however, while this configuration is part of our test suite (we sweep from 1-16 at least with distributed), it is not part of the recipes used to compute coverage

The test that previously hit this was the force_prepare test, which got deleted as this option no longer exists.

The new test will force this

[GiudGiud]

wouhou (reported) coverage increased now

@lindsayad any other concerns?

[lindsayad]

Nice. It would almost be nice to create #ifdef MOOSE_ENABLE_DEPRECATED guards like we have for libMesh that could be used to remove deprecated code from coverage testing for example. What would you think about that @loganharbour ? Probably would make the code more ugly than it's worth to make coverage appear (a little bit) better

[GiudGiud]

thanks!

[loganharbour]

It would almost be nice to create #ifdef MOOSE_ENABLE_DEPRECATED guards like we have for libMesh that could be used to remove deprecated code from coverage testing for example.

I think I'm on board with this. I think it would make maintenance easier as well by just doing a quick grep for that guard

[loganharbour]

We would probably need to add functionality for what to do with deprecated parameters, though

[GiudGiud]

On the doc side,

• We dont have anything on the PLN-4005 for what to do with deprecated features.
• We should have docs in "how_to_contribute_to_moose" to explain the few details about what to do when transforming an object, especially changing its parameters
  all I can think of for now

[socratesgorilla]

I do not believe this PR was ready to be merged. First, there are a couple outright issues (duplicate tests, etc) that are present in this PR that shouldn't have gone through, and second, there are more directional and abstract issues that needed to be discussed and addressed as well. It is clear that the comment I wrote explaining my reasoning on the previous PR: #22268 (comment) was not read, as none of the points I brought up are mentioned at all in the comments above. Dismissing my post on hand for minor errors (and easily fixed errors, mind you) has led to absence of real discussion over certain topics that deserved it, and allowed a bad merge due to unilateral decision-making.

I'll start with going over the outright issues with this PR:

• lower_d_block_generator:

  • The new test added, lower_d.i, along with the first_order test in the corresponding test spec is a direct duplicate of ids.i already in the same folder. There are no differences whatsoever in the test. Please explain why there are two tests that do exactly the same thing.

  If, at the very least, you are adamant that the requirement text saying "this is testing first order elements" stays, then the requirement text on the ids test should have changed, rather than adding a duplicate test. It does not meet the threshold for a unique test.

• sideset_around_subdomain_generator/tests:

  • New test added, [adjacent_to_boundary] seems to come out of nowhere. The only feature or code coverage from the corresponding mesh_modifiers tests it preserves are the parallel capabilities of the generator, and it's unclear why the test exists over something that runs faster and merely tests the parallel capabilities. Can you elaborate on its inclusion? As far as I can tell, there are no lines in the actual source file that make any distinction whatsoever over the requirement specified in the test. Moreover, it does not even cite an issue.

Moving on, however, we need to get into the bigger argument of why we should include or remove tests in MOOSE in the first place.

After going over what is a valid requirement with Cody and Casey over the course of several hours, I'll go over the more high-level problems that I believe are still valid. Some of this will be a reprisal of my earlier post on my PR, so apologies in advance. Let me start by stating two things about what constitutes a unique test requirement by our standards.

First, plainly, is that tests should be unique, i.e. they should be testing a unique feature, function, error pathway, etc. It's important to remember that our requirement testing is not a substitute for unit testing. If there are no unique features being tested, the tests shouldn't be there.

The second would be the concept of, for lack of a better term, encapsulated or implied requirements. Let's use an example to look at this: say that I'm developing a new baseball bat and it needs to not break under 4000 N of force. If that's already a requirement, then something like "the left side of the bat should not break under 4000 N of force" is superfluous. The system already specifies that it shouldn't break under 4000 N, ergo the component pieces of the system would inherently not break as well. The "left side should not break" is a completely implied requirement already, and doesn't pass the barrier for a unique test.

The same can be said for tests here in MOOSE. Take for example, a test spec for a FoobarGenerator. The first test in the spec has a requirement along the lines of "the foobar generator should generate foobars." Under this scenario, a test such as "FoobarGenerator should work on a 2D mesh" is not a unique test, as it is implied by the requirements of MOOSE itself. The notable exception to this is if the code in question does have to do unique things to satisfy that requirement, or in other words, if it has specific unique requirements for the test.

Now, bear in mind, both of these should be taken with the understanding that they are standards applied "in general." There are always exceptions, and always will be. However, it's important that the tests overcome the uniqueness barrier, and I don't believe everything here is, and I don't believe I'm the only one who believes this either.

• First of all, there were 10 tests in the block_deleter folder that were brought over without much thought whatsoever. Here they are, with their details:

  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, 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, if anything, is the contract() and prepare_for_use() functions in libMesh, but nothing specific to the requirements of the BlockDeletionGenerator. They aren't testing any particular functionality of the block deletion and while maybe some of them deserved to stay, I think it is clear that not all of these tests are meaningful. Please refer back to the idea of implied requirements here, especially. It's also important to note that the issues cited in these tests are just a link to Deprecate MeshModifiers and remove tests - push on the apps to do the same. #13814, and have nothing to do whatsoever with the BlockDeletionGenerator.

• The same can also be said for some of the tests in ElementSubdomainIDGenerator. First of all, we already have many tests that run ElementSubdomainIDGenerator on a QUAD4 mesh, as part of other stuff they are doing. All of these tests pass, so we know for certain that an ElementSubdomainIDGenerator is ran on a QUAD4 mesh. I don't believe that "ElementSubdomainIDGenerator should run on a quadrilateral mesh" is a unique requirement. MOOSE as a system already has a functional requirement that the system will support QUAD4 meshes, making this a non-unique requirement. The larger, system-wide requirement implies and encompasses the second completely. Since ElementSubdomainIDGenerator does not do anything special in the code for these meshes, it makes no sense to test for them. They should always be capable of doing this, it isn't a meaningful unique requirement.

The rest of my complaints would be complete retreads of things in my PR's comment, so I won't go over them here. However I would really like these problems to be addressed by @GiudGiud

Regardless, a discussion should have been had on this before merging, and should still be had, and this dismissive behavior benefits no one, and actively hurts the project.

[friedmud]

Locking this until we have time to have a proper meeting about testing so we can have a better discussion.