On 3/18/22 11:15, Mark Dilger wrote:
>
>> On Mar 18, 2022, at 7:16 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
>>
>> This is great, thank you for doing this. I didn't even realize the OAT
>> hooks had no regression tests.
>>
>> It looks good to me, I reviewed both and tested the module. I wonder
>> if the slight abuse of subid is warranted with brand new hooks going
>> in but not enough to object, I just hope this doesn't rise to the too
>> large to merge this late level.
The core code is extracted from a current CF patch, so I think in
principle it's OK.
I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.
> The majority of the patch is regression testing code, stuff which doesn't get installed. It's even marked as
NO_INSTALLCHECK,so it won't get installed even as part of "make installcheck". That seems safe enough to me.
>
> Not including tests of OAT seems worse, as it leaves us open to breaking the behavior without realizing we've done
so. A refactoring of the core code might cause hooks to be called in a different order, something which isn't
necessarilywrong, but should not be done unknowingly.
>
Yes, and in any case we've added test code after feature freeze in the past.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com