On 3/18/22 11:15, Mark Dilger wrote: > >> On Mar 18, 2022, at 7:16 AM, Joshua Brindle <joshua.brin...@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 > necessarily wrong, 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
