NACK. t_basic, and multi-program tests, should be fine. They are easier to read, and allow for debugging using traffic sniffers if needed. I'd honestly prefer one of the following:

1. Separate commands to latch onto the first or the second (or both)
2. Launching one in the foreground and one with gdbserver (file is probably better than port for this)
3. Launching both with gdbserver in the above fashion

Note that we need to be sure that any processes launched in this fashion are killed off when runtests.py finishes.

Keep in mind also that we do already have multiple programs running at all times in the form of the gssproxy daemon and its client(s). (I would also like to see an option for attaching to the gssproxy daemon itself; see above.)

• Test failures should cause a message to be printed about how to debug that test case.
• In the future (may or may not be in scope for this PR) we may want options for just running that one test case.
• There should be a way to pass in flags to make check (e.g., through variables, like make check DEBUG=16 or so).
• I'd like to see an option for running the full valgrind leak checker
• Test suite should indicate at the start of the run that it has flags at all, and how to get the full set of options
• I don't understand the purpose of --noisy-timeout.

(Please comment when you force-push; pagure doesn't generate an event for that.)

Thanks for the feedback :)

• I'll implement that.
• I'll see how that goes; if its easy, I'll bundle it into this PR.
• make check flags, iirc, aren't supported by krb5. At this time, after running an appropriate make, we can directly call python3 tests/runtests.py with the appropriate options we wanted. However, I'll take a look at that.
• I'll add an option for controlling valgrind flags, and an option for forcing valgrind on all test cases (not just gssproxy)
• Okay
• Noisy Timeout is more of an enhancement in the style of the other pull request technically; its meant to make timing out explicitly clear to new runners of the testsuite. Printing returned value == None to the runners doesn't explicitly say that the test timed out without them reading the comments in the code.

• make check CHECKARGS="--debug-num=4 --force-valgrind" is the how to run the test suite with args now

• Sets of tests can be run individually now. So, e.g., python3 tests/t_acquire.py works, along with all of the options under python3 tests/runtests.py. Note that numbering for test cases is per-running-unit, so the test case number under python3 tests/runtests.py differs from python3 tests/t_basic.py for instance.

• I'd like the message printed at the start to use CHECKARGS. make check is useful here because it will update dependencies and I don't want to deal with debugging when they're not. There's therefore no need for the instructions on how to run it "directly".
• Similarly, use make check CHECKARGS= in the debug messages. (If this really offends you, I'm also okay with just removing the "to runtests.py" part.)
• Please remove noisy timeout. If you want, you can print something more useful when tests time out and failure was not expected; otherwise, timeout isn't interesting.
• While looking for valgrind output from --force-valgrind, I noticed that the test numbers as displayed do not match the numbers of the log files. Please reconcile the two.

• make check should now be listed everywhere
• make check CHECKARGS='--debug-num=<x>' now listed on error
• --noisy-timeout removed, but info message printed still
• All logs are per-test case now. Again, numbering is not stable when running only portions of the test suite. Also, short of lovely hackery to preserve the t_.py name, I've decided to go with the simpler $testdir/test_<x>.log naming scheme.

• Shorten the message on initial start. Two of the lines are currently redundant, and an earlier requested removal has not been carried out.
• Per our discussion on IRC and comments above, timeout is not interesting unless it was unexpected. (Test 18 generates an expected timeout.)
• In t_basic.py, there are comments added explaining what p1 and p2 are. Remove them please.

Once these are addressed, I have no further feedback, but we should probably get input from @simo here too.

Okay -- updated. :)

Okay, in the course of using it, I realized that python's/argparse's nargs="+" option is incompatible with passing argument-like things. That is, the following all fail:

CHECKARGS='--valgrind-cmd="valgrind --track-origins=yes -v --leak-check=full --show-leak-kinds=all"'
CHECKARGS='--valgrind-cmd="valgrind,--track-origins=yes,-v,--leak-check=full,--show-leak-kinds=all"'
CHECKARGS='--valgrind-cmd=valgrind --valgrind-cmd=--track-origins=yes --valgrind-cmd=-v --valgrind-cmd=--leak-check=full --valgrind-cmd=--show-leak-kinds=all'

(The latter being most desirable if a touch verbose; but it looks to be discussed here)

I propose that we instead make --valgrind-cmd and --debug-cmd a string, and split on either spaces or commas. Thoughts?

(Per IRC discussion: use a shell when calling subprocess to avoid parsing strings in argparse)

@rharwood updated :)

LGTM

(Per IRC discussion: please pass the TERM variable through to gdb so that TUI mode works, and also fix whatever causes breakage on Debian.)

TERM is passed through, and afaict, executable="/bin/bash" fixes the issue on Debian. Also, all of the artifacts in $testdir now are numbered as expected.

Let's actually do this right.

Thanks for fixing those issues. However, the test artifact numbering is still slightly inconsistent - sometimes there's a leading '0', sometimes not. (I'd recommend removing the leading 0, unless you also want to fix the printed numbers.) One additional comment inline.

My apologies -- fixed.

Commit 7917a46e fixes this pull-request

Commit 5a0d5003 fixes this pull-request

Commit 7917a46e fixes this pull-request