On 7 May 2018 at 09:39, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > Thanks for the patch. This looks good to me. But it it seems that the > test coverage related to pseudorefs is still not great. Ideally, all of > the following combinations should be tested: > > Pre-update value | ref-update old OID | Expected result > -------------------|----------------------|---------------- > missing | missing | accept * > missing | value | reject > set | missing | reject * > set | correct value | accept > set | wrong value | reject > > I think your test only covers the lines with asterisks. Are the other > scenarios already covered by other tests? If not, how about adding them? > That would give us confidence that the new code works in all circumstances. Thank you for your comments. I was not able to find much pseudoref-testing. I think what I should do is a patch 1/2 adding the tests you outlined (some will be expected failures), then turn this patch into a patch 2/2. Martin
