Goddard-Fortran-Ecosystem / pFUnit

Parallel Fortran Unit Testing Framework
Other
169 stars 45 forks source link

MAX_LEN_REAL_AS_STRING is a little too small #428

Closed d7919 closed 1 year ago

d7919 commented 1 year ago

In a test case using real128 kinds it is possible for the test driver to fail with

At line 6849 of file /path/to/pfunit-build/src/funit/asserts/AssertReal_0d.F90 Fortran runtime error: End of record

I believe this is because expected_str is declared as character(len=MAX_LEN_REAL_AS_STRING) where MAX_LEN_REAL_AS_STRING = 40 is not quite long enough to contain the desired string.

As an example

module test_proof                                                                                                                                                                             
  use funit                                                                                                                                                                                   
contains                                                                                                                                                                                      

  @test                                                                                                                                                                                       
  subroutine test_proof_real128_broken                                                                                                                                                        
    use iso_fortran_env, only: real128                                                                                                                                                        
    implicit none                                                                                                                                                                             
    real(real128) :: actual, expected                                                                                                                                                         
    actual = -0.12345e-6                                                                                                                                                                      
    expected = -0.12344e-6                                                                                                                                                                    
    @assertEqual(expected, actual)                                                                                                                                                            
  end subroutine test_proof_real128_broken                                                                                                                                                    

end module test_proof 

aborts as above. Changing MAX_LEN_REAL_AS_STRING to 43 sees the test correctly fail with the desired error message:

test_proof_suite.test_proof_real128_broken Location: [test_proof.pf:12] AssertEqual failure: Expected: <-0.123440003108044038526713848114013672E-6> Actual: <-0.123449993338908825535327196121215820E-6> Difference: <-0.999023086478700861334800720214843750E-11> (greater than tolerance of 0.00000000000000000000000000000000000)

FAILURES!!! Tests run: 1, Failures: 1, Errors: 0 , Disabled: 0 STOP Encountered 1 or more failures/errors during testing.

The maximum value required could of course be larger than 43 if the exponent, were longer say.

tclune commented 1 year ago

Agreed. Similar failures can happen under other circumstances. Been a long time since I had to look at this layer - really should not have any hardcoded lengths at all. (Early days allocatable strings were sometimes wonky and I had to use workarounds.)

Just back from a long bit of travel, so might be a week or more before I can dive into this. But if you see a fix already, feel free to submit a PR (onto develop branch please).

d7919 commented 1 year ago

It's difficult to work out the required length of the allocatable string to represent the real data in advance. There are two slightly hacky workarounds that I can think of immediately:

  1. Use a local fixed length string of excessive size, write to this and use len_trim to determine the actual size required to use to allocate strings. Questionable if there's much advantage to copying out from the excessive size into the allocatable character.
  2. Attempt to allocate the string to a reasonable size, use the eor option to write to detect if the string is insufficient in size and reallocate to a larger string.

The second would look something like

character(len=:), allocatable :: string
integer :: length

length = 8
10 length = length * 2
if (allocated(string)) deallocate(string)
allocate(character(len=length) :: string)
write(string, '(g0)', eor = 10) e

Edit: My mistake eor is only applicable to read, not write. One can still do something similar, although it's even uglier

use iso_fortran_env, only: iostat_eor
character(len=:), allocatable :: string
integer :: length, iostat

length = 8
iostat = iostat_eor
do while(iostat == iostat_eor)
  length = length * 2
  if (allocated(string)) deallocate(string)
  allocate(character(len=length) :: string)
  write(string, '(g0)', iostat = iostat) e
end do

Note this then hides/ignores any other error condition which arises and one may need additional handling of iostat to check success or otherwise.

tclune commented 1 year ago

Yeah. I've done something similar to this in my logging package where one really has no idea in advance of the size of the strings, as even the format is a run-time entity.