On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@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
