On Tue, 9 Jul 2024 at 07:14, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shlok, Here are my review comments for v16-0002
>
> ======
> src/test/subscription/t/004_sync.pl
>
> > > 5.
> > > Here, you are confirming we get an ERROR when replicating from a
> > > non-generated column to a generated column. But I think your patch
> > > also added exactly that same test scenario in the 011_generated (as
> > > the sub5 test). So, maybe this one here should be removed?
> >
> > For 0004_sync.pl, it is tested when 'include_generated_columns' is not
> > specified. Whereas for the test in 011_generated
> > 'include_generated_columns = true' is specified.
> > I thought we should have a test for both cases to test if the error
> > message format is the same for both cases. Thoughts?
>
> 3.
> Sorry, I missed that there was a parameter flag difference. Anyway,
> since the code-path to reach this error is the same regardless of the
> 'include_generated_columns' parameter value IMO having too many tests
> might be overkill. YMMV.
>
> Anyway, whether you decide to keep both test cases or not, I think all
> testing related to generated column replication belongs in the new
> 001_generated.pl TAP file -- not here in 04_sync.pl
I have removed the test
> ======
> src/test/subscription/t/011_generated.pl
>
> 4. Untested scenarios for "missing col"?
>
> I have seen (in 04_sync.pl) missing column test cases for:
> - publisher not-generated col ==> subscriber missing column
>
> Maybe I am mistaken, but I don't recall seeing any test cases for:
> - publisher generated-col ==> subscriber missing col
>
> Unless they are already done somewhere, I think this scenario should
> be in 011_generated.pl. Furthermore, maybe it needs to be tested for
> both include_generated_columns = true / false, because if the
> parameter is false it should be OK, but if the parameter is true it
> should give ERROR.
Have added the tests in 011_generated.pl
I have also addressed the remaining comments. Please find the updated
v18 patches
v18-0001 - Rebased the patch on HEAD
v18-0002 - Addressed the comments
v18-0003 - Addressed the comments
v18-0004- Rebased the patch
Thanks and Regards,
Shlok Kyal
