Need better tests of system tests

[rljacob]

The discussion in issue #1638 shows that its possible to put changes on master that will break a test without detection, in this case the widely used ERS test.

We need better "tests of system tests". We currently test the ERS.py test by deploying them with a couple of A cases in cime_developer. But if the test itself fails to exercise what its supposed to, while still running, then it will always seem to "pass'. If the models under test with ERS actually break restart, it could be weeks/months before anyone notices.

What we need is a case which by design will fail an ERS test. ERS detecting "fail" is then a pass for that test. May need that for other system tests as well.

[billsacks]

I think the problem was ERP, not ERS.

See also the discussion in #290 - which I guess shouldn't have been closed based on the fact that at least the ERP test is still prone to getting broken in this way. I think I closed it because we couldn't come up with an easy way to test the system tests in this way.

Much of my motivation for creating the SystemTestsCompareTwo infrastructure was to allow us to test that infrastructure extensively (via unit tests) and then build some simple tests on top of that. It feels to me that the tests that most often break are ones that have two different configurations but are not handled via SystemTestsCompareTwo - so require some resetting between each build/run. However, we never came up with a way to handle tests that require a restart using that framework (see #448 and #824 ). This leaves ERP as one of the most error-prone tests, which is unfortunate because it is also one of the most widely-used tests.

[rljacob]

Can testmods include source code mods?

[billsacks]

Can testmods include source code mods?

Yes. But, as discussed in #290, this carries a maintenance cost, since the SourceMods will need to be updated whenever the production code is updated. (As an aside, we've had some discussions about storing SourceMods as diffs rather than the full file, which could then be applied via a patch command. That would help somewhat - but would require developing some infrastructure to accomplish it.)

[rljacob]

I think it might be possible to generate an ERP fail with CIME code: Random numbers in the o-a flux. Since those are also part of CIME, it can all get updated at once.

[billsacks]

I think it might be possible to generate an ERP fail with CIME code: Random numbers in the o-a flux. Since those are also part of CIME, it can all get updated at once.

If I understand your suggestion, that would generate a fail in any test that compares two runs. This would test some aspects of the system test infrastructure, and so could be valuable in some respects. However, this wouldn't test the ERP-specific logic - for example, the bug that was introduced recently in the ERP test would still have gone undetected even with this in place. (This could still be worth doing; I'm just trying to point out what would & wouldn't be covered by this.)

[jgfouca]

@billsacks , I'm thinking this task may not be necessary now that nearly all the key test types use the compare-two infrastructure. Compare-two is well tested, does it include tests for failed comparisons? If so, I'm tempted to close this.

[billsacks]

@jgfouca I agree that this may not be necessary. Have you also checked with @rljacob since he's the one who opened this issue?

However, on this point:

Compare-two is well tested, does it include tests for failed comparisons?

All it's testing right now is that _component_compare_test is actually called at the right time. I was assuming that the logic in _component_compare_test was tested elsewhere.

However: Based on your question, I have decided to go one level further in this testing. I just made some changes (which I'll submit in a PR tomorrow, once scripts_regression_tests finishes) that add unit tests to cover _component_compare_test (success or failure) from system_tests_compare_two.

These new tests will cover the relevant logic in system_tests_compare_two and system_tests_common. However, they will NOT cover the logic in hist_utils: I'm stubbing out the actual call into hist_utils, under the assumption that this is - or should be - covered by other tests.

But I'm not sure if hist_utils is actually covered sufficiently by unit tests. If it isn't, it should be, and maybe that could be a specific issue opened to replace this one. There are some test files in tools/cprnc/test_inputs that could possibly be used for this purpose (pre-staging these files into the appropriate location, or (maybe better) generating files like this on the fly, then calling one of the routines in hist_utils.py).

[rljacob]

Maybe this can be replaced with a bunch of more specific issues. We still don't have a test that could have caught the fail discussed in #1638 : A change to the test's python code turned it in to a test that would always pass. A tests that forces a fail would have noticed that.

[billsacks]

@rljacob While I generally agree with the sentiment of making sure our tests are tested, in particular to make sure they pick up fails: I also feel that, now that most of our non-trivial tests use system_tests_compare_two, these sorts of problems are less likely to occur. (In particular, the error in #1638 involves code that no longer exists with the conversion of erp to system_tests_compare_two.) I feel that the best way to handle this is to have near-100% unit test coverage of the relevant code. I feel good about the unit test coverage of system_tests_compare_two itself, but I suspect we still need additional unit tests of some of the things it relies on (like hist_utils).

[jgfouca]

I agree with @billsacks . I think hist_utils is pretty well-covered in scripts_regression_tests. We do lots of tests that do hist comparisons checking for success and we also do a few that verify failure (diff).