Thanks for figuring this out. In my opinion, there are way too many new test cases in this PR. Shouldn't there actually be one test case enough for this tiny change in the code?

Thanks for figuring this out. In my opinion, there are way too many new test cases in this PR. Shouldn't there actually be one test case enough for this tiny change in the code?

I would vote for testing what we do not test elsewhere, I can reduce the number of tests therefore, but I think quite some combinations are never tested.

In my opinion, there are way too many new test cases in this PR.

I think that we should considerably increase the number of tests, given that many combinations and steps are not tested. While it is hard to have definite numbers, I would consider 500-2000 tests for a project of this size not unreasonable. Of course, it would be necessary to structure our tests in a way to ensure rapid development cycles, e.g. by having a subset of quick tests or similar actions we can elaborate on. Having uncovered features is much more concerning in the longer run, which is why I am very much in favor of adding all tests that address previously untested features (of course avoiding redundancy within the suggested tests).

In my opinion its too complex to test all combinations of parameter in ExaDG. Also the code does not have the complexity that all parameters are coupled and need to be tested in all the combinations. You found that the linear steady case is not tested, right? So why not just test this one?

Also related to your comment, @kronbichler: What I do not understand here is why we discuss the "big philosophical problem of testing" and not the problem we currently have here. It should also be part of the discussion that many many tests take part of our (expensive) development time and require resources for maintenance. Also right now as a reviewer, I actually need to invest too much time into such a tiny change. I am not complaining here, it's just that I believe there are more arguments to the discussion.

As an addition: It might turn out that the if-statement can be removed entirely. Shouldn't we then actually rethink the addition of many new tests here? So as a suggestion for the future: Creating a pull request with changing just the line of the if-statement, briefly discussing this (and whether we need tests for this or not; as I said, I am not sure if we already have the final variant regarding the if-statement; it might be that the code can be "simplified" compared to the previous version), and then continue (based on this discussion) could be more effective overall.

As an addition: It might turn out that the if-statement can be removed entirely. Shouldn't we then actually rethink the addition of many new tests here? So as a suggestion for the future: Creating a pull request with changing just the line of the if-statement, briefly discussing this (and whether we need tests for this or not; as I said, I am not sure if we already have the final variant regarding the if-statement; it might be that the code can be "simplified" compared to the previous version), and then continue (based on this discussion) could be more effective overall.

Yes, this is what I will check: #654 (comment)
The cases tested elsewhere, I will also remove!
Then we can have a look at whats left :)

The if mentioned in #654 (comment) is actually never false, meaning we can simply omit the condition entirely (I checked that with all the tests I had previously implemented).

I removed the testcases using the JacobiPreconditioner, not because it is tested elsewhere, but to reduce the number of tests in our own interest. The combination finite strain + unsteady + multigrid prec is tested in the structure/manufactured applciation's tests, the ExcMessages I fixed are un-fixed now (separate PR).

Now do we want to further remove some tests? I do not see any redundancies. And yes, we can never get a coverage of 100%, but I think it might be useful, since one can always overlook some edge-cases. We can move some of the unsteady tests to the structure/manufacured to have a proper reference solution as well (here, we have a large error, since I cannot make the test cheap == very small number of time steps with the solution in this application).

For me, the reasoning is not clear why to have "redundant" tests e.g. for AMG and "multigrid". Note that "Multigrid" still has many parameters and that "Multigrid" internally contains AMG, see e.g.:

  Multigrid type:                            phMG
  p-sequence:                                Bisect
  Smoother:                                  Chebyshev
  Preconditioner smoother:                   PointJacobi
  Iterations smoother:                       5
  Smoothing range:                           2.0000e+01
  Iterations eigenvalue estimation:          20
  Coarse grid solver:                        CG
  Coarse grid preconditioner:                AMG

How would you motivate the need for this from the code? If I understand the implementation correctly, the Structure module uses a "generic" AMG preconditioner, not a "structure-specific" AMG preconditioner, or do I misunderstand something? Assuming this is correct, the structure-specific parameters "steady/quasistatic/unsteady" and "small/large strain" are not really seen by the implementation of the AMG preconditioner used when running Structure simulations. Then, many tests could be considered superfluous. Generally, I consider it a con of the procedure suggested here that this gives somewhat exponential complexity in the number of tests w.r.t. parameters, and I am not sure if this really helps (given that the main purpose of most of the tests here seems to be "no segmentation fault occurred" while most of the parameter combinations are actually still untested (and where I argue that we cannot avoid this) or while the implementation seems to be somewhat insensitive to the variation of parameters being tested).

In my opinion, added tests will not be removed again in the future (because one might have to expect to do harm if removing them, arguing that the person who introduced them might have had "good reasons to do this"). We are now evaluating "good reasons to do this", hence I would like to ask you for your understanding that I would like to understand the reasoning for adding certain tests.