Consolidate Test suite C++ code/create `templates` directory #4489

jwpeterson · 2015-01-06T21:08:46Z

The discussion for this started in PR #4469. Basically, @permcody pointed out that we are getting a lot of source code in test/src and test/include, some of which is performing very simple tasks or is just redundant. So this issue is mainly to come up with a design that consolidates some of the test code, if possible.

As a side issue, @jwpeterson noted that it is sometimes nice to be able to copy/paste code from the tests directory when starting something new, but it will be more awkward to do this if the testing source code gets consolidated. So it might be worth having a "templates" directory with a template Material, Kernel, etc. in there for people to copy paste from that would also be run as tests to ensure they were kept up-to-date.

aeslaughter · 2015-01-08T14:57:36Z

I think I am on board with creating a test object for each object type (Kernel, etc.), but we need to come up with a common scheme for triggering the different tests. In the few objects I have made I have a MooseEnum parameter, "test_type", that changes the behavior. I would want this to be consistent through all the objects, perhaps we need a simple TestObject base class to bring in this parameter consistently.

👍 on the templates directory, I think this is a great idea. It then would be super easy to even make a little helper script to rename the file and class names for building a new object, something I already do. For example:

moose_object Kernel MyCrazyKernel

friedmud · 2015-01-08T17:06:14Z

I'm going to copy over my comments from the PR:

friedmud · 2015-01-08T17:06:47Z

As for the debate about testing objects... I actually prefer to have a bunch of small testing objects that test individual features. That way when something fails it's easy to track down.

This is actually what MooseTest is for... I don't consider it bloat because it doesn't get compiled unless you specifically go do it.

So far we've done a really good job of using objects from MOOSE itself to do the testing with... but sometimes there is no way around it (I recently had to add an object to test solution vector serialization - nothing in MOOSE was using it).

Also: I would MUCH rather add to MooseTest than add to MOOSE itself...

Finally: If you think of these testing objects like Unit tests then having one object per feature makes sense.... that's what we do in MooseUnit for instance...

friedmud · 2015-01-08T17:06:58Z

Specifically I don't want to see this:

TestAux::execute()
{
  switch(test_type)
  {
    case 0:
      // Test SomeObject
    case 1:
      // Test AnotherObject
    case 2:
      // Test a feature
    case 3:
      // more testing
    case 4:
      // yet another test
  }
}

Yuck!

MOOSE is designed for lots of small objects... and that's the way we should do our testing as well.

friedmud · 2015-01-08T17:07:20Z

(from @aeslaughter )
@friedmud Just to be clear, I don't think we should TestObjects would be in MOOSE, it would still be in tests/src.

However, I generally side with you. Lets see what @permcody says, he was the one that really doesn't like all the objects.

friedmud · 2015-01-08T17:07:32Z

Seriously... the more I think about this... the more it makes me sick to think of monolithic testing objects. Thousands of parameters... hundreds of class member variabes... spaghetti of if statements and case statements.

Just straight up NO.

That said - we should be judicious in creating new testing code... try to reuse what we already have when possible. BUT: Like I said, I think we've been really good about doing this up to now and I don't see any reason to make a policy change.

friedmud · 2015-01-08T17:07:48Z

(from @YaqiWang)

Test codes can currently indicate what is going wrong for us, which is really nice. The size of test codes should not be a big issue due to the duplication in them IMO. Most of these duplication are really stable in MOOSE and will possibly not cause any painful refactoring. We do not need to make test codes fancy. ;-)

friedmud · 2015-01-08T17:08:07Z

@aeslaughter Right: I was just saying that if we start to make rules about what kinds of objects we can put in MooseTest people might prefer to put "testing code" over into MOOSE (there are lots of objects in MooseTest that aren't "testing" code... but also necessarily aren't useful enough to be in MOOSE either)

Things like:
https://github.com/idaholab/moose/blob/devel/test/include/bcs/DGMDDBC.h
https://github.com/idaholab/moose/blob/devel/test/include/bcs/OnOffNeumannBC.h
https://github.com/idaholab/moose/blob/devel/test/include/bcs/PolyCoupledDirichletBC.h
https://github.com/idaholab/moose/blob/devel/test/include/bcs/ScalarVarBC.h

And many more.

Each of those "could" be in MOOSE (arguably it's possible that someone might find them useful)... but we were using them to exercise a bit of code. If we start restricting what goes in MooseTest then that might lead to a few more of these types of objects actually ending up in MOOSE itself...

friedmud · 2015-01-08T17:09:37Z

Ultimately I just don't think this is enough of a "problem" to warrant a heavy handed code / system solution...

permcody · 2015-01-08T17:30:37Z

I'm not necessarily advocating going to one object for everything. I think the right thing to do will be something in the middle. Using your example of an ugly case statement and turning it around to have one object for everything, you get this:

BadStatefulMaterial.h                     DerivativeMaterialInterfaceTestProvider.h OutputTestMaterial.h                      SumMaterial.h
ComputingInitialTest.h                    Diff1Material.h                           SpatialStatefulMaterial.h                 VarCouplingMaterial.h
CoupledMaterial.h                         Diff2Material.h                           StatefulMaterial.h                        VarCouplingMaterialEigen.h
CoupledMaterial2.h                        LinearInterpolationMaterial.h             StatefulSpatialTest.h                     VecRangeCheckMaterial.h
DerivativeMaterialInterfaceTestClient.h   MTMaterial.h                              StatefulTest.h

Diff1Material, Diff2Material?!?
StatefulMaterial, StatefulTest?!?

What I'd like to see is just better design of our testing objects. Consolidation where it makes sense. For instance, we don't need another version of the Diffusion kernel with just one extra thing added to it to trigger an extra line of code. In the last case, Andrew created a whole new object just to test a method that returns a simple string. The compute method of that object was a duplicate of something we already had. Why not just add a method in InitialSetup to trigger the testing of that string? In the case of objects that trigger errors, why not have a case statement?

I agree @friedmud, I don't want a giant case statement in a few monolithic objects, but I don't want several hundred small objects sitting around on my system for testing fewer lines of nonessential code that they themselves contain. Let's be smart about how we consolidate, and when.

friedmud · 2015-01-08T18:31:47Z

I definitely agree that some cleanup is needed. A lot of that stuff is left over from long ago...

My favorite is definitely:

SpatialStatefulMaterial
StatefulMaterial
StatefulSpatialTest
StatefulTest

lol

andrsd · 2015-01-08T19:27:03Z

I'll take care of that Diff{1|2}Material stuff...

On Thu, Jan 8, 2015 at 11:31 AM, Derek Gaston notifications@github.com
wrote:

I definitely agree that some cleanup is needed. A lot of that stuff is
left over from long ago...

My favorite is definitely:

SpatialStatefulMaterial
StatefulMaterial
StatefulSpatialTest
StatefulTest

lol

—
Reply to this email directly or view it on GitHub
#4489 (comment).

Refs idaholab#4489

1) Diff{1|2}Material removed 2) Replaced with a better test that exercises different types of material properties. Refs idaholab#4489

permcody added C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software. labels Jan 6, 2015

aeslaughter mentioned this issue Jan 8, 2015

Fixes access to block/boundary names given material property #4469

Merged

andrsd added a commit to andrsd/moose that referenced this issue Jan 8, 2015

Replacing Diff{1|2}Material with GenericConstantMaterial

2e71ab0

Refs idaholab#4489

andrsd added a commit to andrsd/moose that referenced this issue Jan 8, 2015

Diff{1|2}Material refactored

5c844e6

1) Diff{1|2}Material removed 2) Replaced with a better test that exercises different types of material properties. Refs idaholab#4489

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Test suite C++ code/create `templates` directory #4489

Consolidate Test suite C++ code/create `templates` directory #4489

jwpeterson commented Jan 6, 2015

aeslaughter commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

permcody commented Jan 8, 2015

friedmud commented Jan 8, 2015

andrsd commented Jan 8, 2015

Consolidate Test suite C++ code/create templates directory #4489

Consolidate Test suite C++ code/create templates directory #4489

Comments

jwpeterson commented Jan 6, 2015

aeslaughter commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

friedmud commented Jan 8, 2015

permcody commented Jan 8, 2015

friedmud commented Jan 8, 2015

andrsd commented Jan 8, 2015

Consolidate Test suite C++ code/create `templates` directory #4489

Consolidate Test suite C++ code/create `templates` directory #4489