Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework Fortran error handling to support unit tests #140

Closed
billsacks opened this issue Apr 21, 2016 · 8 comments
Closed

Rework Fortran error handling to support unit tests #140

billsacks opened this issue Apr 21, 2016 · 8 comments

Comments

@billsacks
Copy link
Member

This isn't something needed immediately, but it would be very nice to have at some point, to make the pFUnit-based unit tests more useful. I'm opening this issue as a place to record some thoughts:

It would be good to rework the Fortran error handling code (currently, basically shr_sys_abort) so that you could plug in a different error handler when doing unit tests. For example, you could plug in an error handler that throws a pFUnit exception.

This would serve at least two purposes:

(1) Currently, there is no good way to test error-handling code: If you try to have a unit test that triggers an abort, then your whole test suite dies. I believe (but I'm not 100% sure) you could handle this if you swapped out the production error handler for an error handler that raised a pFUnit "exception", then checked to make sure that the expected exception was raised.

(2) If there's a bug in either your production code or your test code that unintentionally causes shr_sys_abort to be triggered, then that whole suite of pFUnit tests aborts, and you aren't given very a very useful error message (e.g., it doesn't even point to the test that failed). If you could plug in a different error handler, which raised a pFUnit "exception" rather than aborting, then you would see useful information about which test(s) failed, and other tests would continue.

@quantheory went a long ways down the path of doing this two years ago. I can't remember why this never quite made it to the trunk. @jedwards4b also sent out an open source Fortran error handler recently, which I haven't looked at closely.

@jedwards4b
Copy link
Contributor

@billsacks
Copy link
Member Author

I realized that much of this benefit could be achieved by using Sean's stub version of shr_sys_mod: shr_sys_mod.nompi_abortthrows.F90.

@billsacks
Copy link
Member Author

billsacks commented Jul 27, 2017

I have some ideas for how to make this work that I'll work on today. It amounts to doing something similar to Sean's shr_sys_mod.nompi_abortthrows.F90, but without requiring fully replacing shr_sys_mod.

I can see 2 solutions:

(1) Use a preprocessor macro, like TESTING_WITH_PFUNIT, which will be defined in unit test builds

Then shr_sys_abort will have:

#ifdef TESTING_WITH_PFUNIT
   use pfunit
#endif

and, at the start of shr_sys_abort:

#ifdef TESTING_WITH_PFUNIT
    throw()
#endif

Don't have an #else. That way it's clear that we're not avoiding the abort stuff ever - we're just doing an additional thing first. (Point is: I don't want to accidentally miss doing the true abort in production code.)

(2) Move shr_sys_abort to its own file (shr_abort_mod). shr_sys_mod will use shr_abort_mod, with:

use shr_abort_mod, only : shr_sys_abort => shr_abort

so that existing code doesn't need to be changed.

We can have a different version of shr_abort_mod.F90 that is built for all tests – e.g., shr_abort_throws_mod.F90. This defines its own shr_abort routine that throws a pfunit exception rather than actually aborting.

@billsacks
Copy link
Member Author

Jim Edwards prefers option (1) above, so I'll give that a shot. He also requests that the msg that's passed into shr_sys_abort be passed along to the 'throw' if possible, so that we can make sure we're aborting for the right reason.

@gold2718
Copy link

I would like to see @jedwards4b reasoning written down here as part of this discussion. My feeling is that avoiding ifdefs in the code where possible is preferable and therefore I like option 2 as a cleaner option.
I feel option 2 is more similar to how we treat other interfaces whose meaning changes at compile time. One (of many) examples would be the duplicate interfaces of DATM and CAM.

@jedwards4b
Copy link
Contributor

Option 2 is fine, it just looked like more work to me.

@billsacks
Copy link
Member Author

Okay, I'll go with option 2. I think it's a similar amount of work.

@billsacks
Copy link
Member Author

@jedwards4b turns out your intuition was better than mine: option 2 has ended up being a fair amount of work... I'm getting close, though....

jedwards4b added a commit that referenced this issue Jul 28, 2017
Facilitate expected error unit tests
Pull shr_sys_abort into its own module - now call shr_abort_abort in
shr_abort_mod. (The routine name, while redundant, follows the share
code convention of MODULENAME_VERB.) Other code can still use this via
shr_sys_mod: shr_sys_abort, via a rename done in that module. Two other
routines from shr_sys_abort that are used by shr_abort_abort are also
moved into shr_abort_abort to avoid circular dependencies.

There are no substantive changes in shr_abort_mod relative to the code
that used to be in shr_sys_abort: I have just moved code into this new
module and done some reformatting.

The purpose of this separation was to facilitate using a replacement
abort routine in pFUnit-based unit tests. This PR also adds a module
thath provides that replacement, which throws a pFUnit exception rather
than actually aborting. This also changes the cime unit test build so
that this new pFUnit-based abort routine is included in all drv unit
tests.

This also adds a simple unit test of the pFUnit-based abort
routine. This demonstrates how you can use this to write your own
"expected failure" unit tests:

  @test
  subroutine test_abort(this)
    class(TestShrAbort), intent(inout) :: this

    call shr_abort_abort('Test message')
    @assertExceptionRaised('ABORTED: Test message')
  end subroutine test_abort
Test suite:

scripts_regression_tests on yellowstone and cheyenne
cime fortran unit tests with gnu on my Mac
SMS.f09_g16.X and SMS_D.f09_g16.X with:
hobart_nag
hobart_pgi
yellowstone_intel
cheyenne_gnu
SMS.f09_g16.X and SMS_D.f09_g16.X with cheyenne_intel, with a
shr_sys_abort call inserted: made sure runs still die properly when
they call shr_sys_abort
Test baseline: N/A
Test namelist changes: none
Test status: bit for bit
Fixes #140

User interface changes?: none

Update gh-pages html (Y/N)?: N

Code review:

@ekluzek @bandre-ucar - this could be useful for adding expected error
unit tests in CLM
@ghost ghost removed the Assigned label Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants