On 11/8/19 4:40 PM, Mark Dilger wrote: > > > On 11/8/19 9:22 AM, Andrew Dunstan wrote: > ... >> This will need to be rewritten in light of the above, but see >> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a...@2ndquadrant.com> >> > > Thanks for the reference. Having read your motivating example, this > new review reverses some of what I suggested in the prior review. > > > In the existing TestLib.pm, there are eight occurrences of nearly > identical usages of IPC::Run scattered through similar functions: > > [snip]
> > One rational motive for designing TestLib with so much code > duplication is to make the tests that use the library easier to read: > > command_like_safe(foo); > command_like(bar); > command_fails_like(baz); > > which is easier to understand than: > > command_like(foo, failure_mode => safe); > command_like(bar); > command_like(baz, failure => expected); > > and so forth. > > In line with that thinking, perhaps you should just create: > > command_fails_without_tty_like(foo) > > and be done, or perhaps: > > command_fails_like(foo, tty => 'closed') > > and still preserve some of the test readability. Will anyone like the > readability of your tests if you have: > > command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ? > > Admittedly, "foo", "bar", and "baz" above are shorthand notation for > things in practice that are already somewhat hard to read, as in: > > command_fails_like( > [ 'pg_dump', 'qqq', 'abc' ], > qr/\Qpg_dump: error: too many command-line arguments (first is > "abc")\E/, > 'pg_dump: too many command-line arguments'); > > but adding more to that cruft just makes it worse. Right? > OK, I agree that we're getting rather baroque here. I could go with your suggestion of YA function, or possibly a solution that simple passes any extra arguments straight through to IPC::Run::run(), e.g. command_fails_like( [ 'pg_dump', 'qqq', 'abc' ], qr/\Qpg_dump: error: too many command-line arguments (first is "abc")\E/, 'pg_dump: too many command-line arguments', '<pty<', \$eof_in); That means we're not future-proofing the function - we'll never be able to add more arguments to it, but I'm not really certain that matters anyway. I should note that perlcritic whines about subroutines with too many arguments, so making provision for more seems unnecessary anyway. I don't think this is worth spending a huge amount of time on, we've already spent more time discussing it than it would take to implement either solution. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
