Portable test suite

Rather than introduce more env vars:

83 + if ($ENV{PERCONA_TOOLKIT_TEST_NORMALIZE_CONNECTION_IPS}) {
84 + $cxn_string =~ s/127.1/127.0.0.1/g;
85 + }

Let's just have one: PTTEST. Then the modules can do things like use IP instead of hostname, etc. to help make tests consistent.

As for 127.1 vs. 127.0.0.1, we should just fix the root of the problem: use one or the other consistently in the tests.

/* BRIAN: We talked about this on IRC, but to recap, that's needed for Cygwin and possibly Windows. I'm fine with removing it and s/\Q127.1/127.0.0.1/g in one commit*/

Use English:
    155 + local $/;

/* BRIAN: Yep, my bad. Sandbox/... also doesnt' use English, but that has a reason */

Will newer Perl's "use of uninitialized value" here if $pid is undef?
    160 + chomp($pid);

/* BRIAN: (irc)*/

What's all this for?

 523 + my $replace = q{@@SQL_MODE};
    524 + for my $mode ( qw(ANSI_QUOTES ANSI DB2 MAXDB MSSQL ORACLE POSTGRESQL 524 ) ) {
    525 + $replace = "REPLACE($replace, '$mode', '')"
    526 + }
    527 + $replace = qq{REPLACE($replace, ',,', ',')};

/* BRIAN: Bug fix for a customer, see https://code.launchpad.net/~percona-toolkit-dev/percona-toolkit/fix-i26211-1058285-821722-implicit-ansi_quotes */

Should $ARGV in the string be just ARGV?
    814 + close ARGV or die "Can't close $ARGV: $OS_ERROR";

/* BRIAN: $ARGV contains the current filename, so that should be better. However, I hadn't intended to push that yet! That's half-finished. Thanks for catching it, I'll be reverting that soon. */

Is this a bug fix?
   1870 - = $o->get('dry-run') ? $o->get('chunk-size') * $limit
   1871 - : $chunk_time ? $avg_rate * $chunk_time * $limit
   1872 - : $o->get('chunk-size') * $limit;
   1873 + = $o->get('dry-run') ? $o->get('chunk-size') * $limit
   1874 + : $chunk_time && $avg_rate ? $avg_rate * $chunk_time * $limit
   1875 + : $o->get('chunk-size') * $limit;

Related to that ^,

   1895 + PTDEBUG && _d("Something might be wrong with gettimeofday() 1895 ",

We have talked about a SystemSanityCheck module (or something like that) that would test known weirdnesses and either adjust for them or cause the tool to bail out. Perhaps it's time we do this and put a check for this in there.

/* BRIAN: Yeah, see https://bugs.launchpad.net/percona-toolkit/+bug/1050737
   That sounds good to me. If anything, the above changes can be reverted (grep the commit log for Gentoo)
*/

Can remove the last sentence:

3050 + . "in the slave cluster. Feel free to contact us to "
3051 + . "explain your use case if you want this supported.\n"

/* BRIAN: Will do. */

Why this?
  3086 - $total_time += $tbl->{nibble_time};
  3087 + $total_time += $tbl->{nibble_time} || 1;

/* BRIAN: Also 1050737 */

Need to revert this:

   7085 - eval $TEST_CMD
   7086 + $TEST_CMD

I.e. the "eval" is required so TEST_CMD can have spaces.

/* BRIAN: I don't recall doing that change at all, so sure. */

I don't think these are a win:

__create_stop_or_start_file(), _create_use_file(), _create_config_file(), etc.

Using a program to create a program is messy and prone to weird errors and subtle, hard-to-find bugs. I think we need to restore the script templates (start, stop, use) and the actual config files (which were templates where s/PORT/12345/ for example), and simply append to the config files various startup options like skip-innodb, etc. -- This is beginning to show why Bash is better than Perl in some cases because in Perl we have to do a much more long, complex song and dance. When what we're doing is primarily tasks for which there are shell commands, then we should tend towards Bash.

/* BRIAN: I disagree about use/stop/start; The gen'd programs are extremely simple and only used manually by us -- there can't be subtle bugs because no other code depends on them. And there's two wins here: Portability and code reuse. The gen'd scripts Just Work everywhere (..with perl), and they use the same code paths as the test suite in general, so there's no extra executable with it's own quirks.
Restoring the config files is alright by me though. I actually removed them earlier today because I had mistakenly not been using them at all for at least a hundred commits (the code that gen'd them was checked if they existed, rather than if they DIDN'T exist. bluh)
*/

I think we need to make it policy to always diag system commands:

   8443 -diag(`touch $pid_file`);
   8444 +touch $pid_file;

   8502 -diag(`rm $tempfile >/dev/null`);
   8503 +unlink $tempfile;

   8853 -diag(`rm -rf $tmpdir/*`);
   8854 +remove_tree $tmpdir;
   8855 +make_path $tmpdir;

If some test failes and $pid_file doesn't exist, then touch $pid_file will print something to STDERR that will show up as an error.

/* BRIAN: True, that's a big oversight from me. Luckily, of the above, only unlink would need changing, since the other three throw exceptions if there's any errors. I didn't know that the touch command threw warnings at all, except for "disk full" */

I think t/lib/Pingback.t is going to merge conflict because I changed the post => args to fix sorting issues on some platforms.

Why do some PerconaTest subs now have prototypes?
   sub throws_ok (&$;$) {

Is really necessary?

  10500 + # use & to ignore the prototype
  10501 + &throws_ok($test->[0], qr/$test->[1]/, $test->[1]);

I thought &sub was deprecated long ago.

/* BRIAN: Not particularly necessary, I just didn't like the disparity between Test::Exception / Test::Fatal and our own throws_ok -- the "good practice" for test functions is to give them prototypes, which sounds silly to me but that's how it is.
ALso, nah, &sub is good, but you should only use it if you know what you're doing with it, that's why people are told not to use it
 - &sub(...) calls sub without checking the prototype
 - &sub calls sub and makes visible the current @_ (changes to @_ in sub will propagate to the parent)
*/

This seems overkill:
   (undef, undef, $output) = cmd_output($cmd);
From my grep, it looks like we throw away the first 2 retvals most of the time, so why have them at all? Also, is it absolutely necessary to fork?

/* BRIAN: Horrible, horrible misdesign, and guilty as charged. Yes, we can throw those away, I should've done it a long time ago. Around halfway through For what it's worth, I noticed my screwup and made sure that they can be easily changed with sed.
It's not really forking persay, it's using IPC::Open3. It was the only sane option to avoid shelling out, although we *could* use IPC::Cmd::run, which I'm using on the Sandbox libs and works marvelously, however, we lose the ability to send things to the child's STDIN.
*/

Related:
   12950 +my ($in, $out, $err, $pid) = PerconaTest::_run_cmd($cmd,
Now the underlying ("private") _run_cmd() is being used?

/* BRIAN: Ah, thanks for bringing this up, I need to write a longish mail about it. We can probably revert that; It's an experiment from the time I wanted to introduce run_in_background(), but I'll get to that */

English:

  12718 +(undef, undef, $output) = cmd_output($^X, '-I', catfile($trunk, $samples),

/* BRIAN: On it. Couple of other spots also use $^X, I'll fix those as well */

What's this doing?

  12962 +my @err = <$err>;
  12963 +diag(@err) if @err;
  12964 +
  12965 +close $err;
  12966 +close $out;
  12967 +waitpid($pid, 0);

I think it's reading STDERR from some earlier forked command? Then waitpid() due to forking?

/* BRIAN: Yes. THat's from t/pt-archiver/check_slave_lag.t: it starts a pt-archiver run on the background, and at the end prints anything printed to STDERR. But see the above about run_cmd_background */

What's the #> for?

  13094 + '--where', "pub_date < '2010-02-16'", qw(--bulk-delete --limit 2)); #>"

  13114 + '--where', "pub_date < '2010-02-16'", qw(--bulk-delete --limit 100)); #>"

/* BRIAN: Oh, bugger. That's my text editor screwing up .t files. I thought I had removed all of those. */

I could be mistaken but with:

  14679 + local $ENV{PTDEBUG}=1;
  14680 + cmd_output($cmd, "F=$cnf,D=test", qw(--clear-deadlocks test.make_deadlock));

I thought subprocesses received a new, empty environment. So even if the parent sets PTDEBUG=1, the subproc won't have that.

/* BRIAN: Hm, let me check...
hugmeir@naw:~$ perl -MIPC::Open3 -E 'local $ENV{PTDEBUG}="testing"; open3(my ($in, $out, undef), q{echo $PTDEBUG}); say <$out>'
testing

Works for me. I suppose that it always does, otherwise our previous way of starting sandboxes with backticks wouldn't have worked either?
*/

Shouldn't these be TODO un-shellify also:

  14826 +$output = `ps -eo pid,args | grep '$cmd @args \-\-dest ' | grep -v grep`;

/* BRIAN: Yes and no. Yes, I wish, but all the spots that use ps in the tests use --daemonize, and I'll get to that in the run_cmd_background thing */

Going from 1 line to 10 is a questionable win. I can't imagine the Perl would be faster, either:

  15220 - sed => ["'s/ (/ (/g'"],

  15223 + transform_result => sub {
  15224 + my ($file) = @_;
  15225 + open my $tmp_fh, q{<}, $file
  15226 + or die "Cannot open $file: $OS_ERROR";
  15227 + while (<$tmp_fh>) {
  15228 + s/ \(/ (/g;
  15229 + print;
  15230 + }
  15231 + close $tmp_fh;
  15232 + },

/* BRIAN: Cheeky answer: I can take that down to one! (untested)
transform_result => sub { print slurp_file(shift) =~ s/ \(/ (/g },

We could do that. Honestly, all I was thinking when writing that is removing a bunch of nearly-unused code (one of the few locations that use sed => ...) from no_diff(), not really the efficiency.
*/