On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch!
>
> > I have added a test case for non-dry-run mode to ensure that
> > replication slots, subscriptions, and publications work as expected
> > when '--all' is specified. Additionally, I have split the test file
> > into two parts:
> > 1) Command-line sanity checks – Validates the correctness of option parsing.
> > 2) '--all' functionality and behavior – Verifies the health of
> > subscriptions and ensures that --all specific scenarios, such as
> > non-template databases not being subscribed to, are properly handled.
> > This should improve test coverage while keeping the structure manageable.
>
> TBH, I feel your change does not separate the test file into the two parts. ISTM
> you just added some validation checks and a test how --all option works.
> Ashutosh, does it match your suggestion?
>
Thanks Hayato for pointing it out. The test changes don't match my
expectations. As you rightly pointed out, I expected two (actually
three if needed) separate test files one for argument validation and
one for testing --database scenarios (the existing tests) and one more
for testing same scenarios when --all is specified. Right now all it
does is "# Verify that only user databases got subscriptions (not
template databases)". I expected testing the actual replication as
well like tests between lines around 527 and 582 in the latest
patchset. Those tests are performed when --database is subscribed. We
need similar tests performed when --all is specified. I didn't find
any of those tests being performed on node_s2. Given that the tests
for --databases and --all will be very similar, having them in the
same test file makes more sense. We also seem to be missing
$node_s2->teardown_node, do we?
--
Best Wishes,
Ashutosh Bapat
