Errors in system test's __init__ method not reported correctly

[billsacks]

I noticed that, if I raise an exception in the init method of a system test, the test gets reported as a PASS.

Example:

--- a/utils/python/CIME/SystemTests/sms.py
+++ b/utils/python/CIME/SystemTests/sms.py
@@ -15,3 +15,4 @@ class SMS(SystemTestsCommon):
         initialize an object interface to the SMS system test
         """
         SystemTestsCommon.__init__(self, case)
+        raise RuntimeError

Then

./create_test --test-root . SMS.f10_f10.ICLM45.yellowstone_gnu

Results in:

Finished SHAREDLIB_BUILD for test SMS.f10_f10.ICLM45.yellowstone_gnu in 22.090373 seconds (FAIL)    Case dir: /glade/scratch/sacks/cesm_code/cesm2_0_alpha01_v5/cime/scripts/SMS.f10_f10.ICLM45.yellowstone_gnu.20160811_214325
At test-scheduler close, state is:
FAIL SMS.f10_f10.ICLM45.yellowstone_gnu (phase SHAREDLIB_BUILD)
    Case dir: /glade/scratch/sacks/cesm_code/cesm2_0_alpha01_v5/cime/scripts/SMS.f10_f10.ICLM45.yellowstone_gnu.20160811_214325
test-scheduler took 45.6888129711 seconds

But TestStatus has:

PASS SMS.f10_f10.ICLM45.yellowstone_gnu CREATE_NEWCASE
PASS SMS.f10_f10.ICLM45.yellowstone_gnu XML
PASS SMS.f10_f10.ICLM45.yellowstone_gnu SETUP

and running cs.status gives:

SMS.f10_f10.ICLM45.yellowstone_gnu (Overall: PASS), details:
  PASS CREATE_NEWCASE
  PASS XML
  PASS SETUP

I am particularly thinking about this with respect to the clone-based test infrastructure: I have set this up to do all test-specific setup from the constructor. If there's no way to address this issue (and thus errors in the constructor will lead to tests looking like they have passed), then I should probably move this test-specific setup into the build_phase method instead. But it seems like it's worth addressing this issue for more general reasons, if possible.

[jgfouca]

Good catch.

[billsacks]

@jgfouca : you probably have some thoughts on how to fix this, but I'll share the somewhat naive possible ideas that came into my head during my walk home:

1. Wrap the test's init call in a 'with TestStatus' context manager, similarly to what's done for case_setup
2. At the beginning of the SystemTestCommon init method, it creates its TestStatus object, then sets some TestStatus to FAIL - e.g., for a TestInit phase. This status remains FAIL until one of the test's public methods (build or run) is called, at which point it's clear that TestStatus succeeded, so this status can be set to PASS. This would work as long as the individual test's constructor called SystemTestCommon.init before it did any work itself (which seems like a good general object-oriented programming rule). (Note that you cannot set this status to PASS at the end of SystemTestCommon's init method because the individual test may still do more work in its constructor after SystemTestCommon.init returns.)
3. If the main problem is with respect to my new infrastructure, then maybe I can do some kind of 'with TestStatus' thing in SystemTestCompareTwo.init. But this won't help other tests, nor would it help with errors raised in the SystemTestCommon.init constructor itself (such as an error in setting up the LockedFiles).

Of course, some of these may not work, and you should feel free to ignore these if you have your own thoughts :-)

[jgfouca]

Looking more closely at this, it's hard to see a good solution. My opinion is that an exception escaping from a test constructor is a programming error, so don't expect things to be in a consistent state.

The core problem is that, at the time of construction, the Test object does not know which phase is about to be run, so a preemptive FAIL is impossible. Even if it weren't, a preemptive FAIL would not be safe because any instances of wait_for_tests waiting on that test would interpret that as a completed test.

[billsacks]

@jgfouca : Let me know what you think about the solution I propose in PR #391