* [PATCH] test-lib: save test counts across invocations
@ 2011-09-01 13:08 Thomas Rast
2011-09-01 16:14 ` Junio C Hamano
2011-09-01 16:38 ` Jeff King
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Rast @ 2011-09-01 13:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
Under 'prove' we can get progress status for tests running in
parallel. However we never told it the total number of tests in a
file in advance, and thus the progress only showed the tests already
executed.
Save the number of tests run ($test_count) in a file under
test-counts/. Then when sourcing test-lib.sh the next time, compare
the timestamps. If the counts file is older than the test, discard.
Otherwise use the count that we saved and give prove the test plan
("1..N") up front.
This results in 'make prove' giving progress output like
===( 2884;121 8/12 24/147 1/8 1/3 )============================
if you have already run the tests before.
Prerequisite changes can mean that a whole test group is skipped
(e.g. NO_SVN_TESTS=1). We thus need to be somewhat careful to only
emit the "full" plan once we know we're not going to $skip_all.
t9700 needs special treatment on top of $test_external_has_tap because
the latter can only be set once we know that the external test will
run. If a prerequisite fails, we still need to emit the plan.
The Makefile changes are required because we want to keep that
test-count subdirectory unless the *user* invokes 'make clean', but we
previously ran the latter ourselves after every successful test run.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Sparked by a discussion on G+. I think this is the "simple" approach.
The "cute" approach would be to let test-lib.sh define test_* as
test-counting dummies once, source the test script itself (avoiding
the sourcing loop with test-lib) to count what it does, then do the
real work.
t/.gitignore | 1 +
t/Makefile | 9 ++++++---
t/t9700-perl-git.sh | 1 +
t/test-lib.sh | 27 +++++++++++++++++++++++++--
4 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/t/.gitignore b/t/.gitignore
index 4e731dc..7de845f 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -1,3 +1,4 @@
/trash directory*
/test-results
+/test-counts
/.prove
diff --git a/t/Makefile b/t/Makefile
index 9046ec9..c70de07 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -36,11 +36,14 @@ $(T):
pre-clean:
$(RM) -r test-results
-clean:
+post-clean:
$(RM) -r 'trash directory'.* test-results
$(RM) -r valgrind/bin
$(RM) .prove
+clean: post-clean
+ $(RM) -r test-counts
+
test-lint: test-lint-duplicates test-lint-executable
test-lint-duplicates:
@@ -55,7 +58,7 @@ test-lint-executable:
aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
- $(MAKE) clean
+ $(MAKE) post-clean
aggregate-results:
for f in test-results/t*-*.counts; do \
@@ -111,4 +114,4 @@ smoke_report: smoke
http://smoke.git.nix.is/app/projects/process_add_report/1 \
| grep -v ^Redirecting
-.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report
+.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report post-clean
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 3787186..20ec097 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -4,6 +4,7 @@
#
test_description='perl interface (Git.pm)'
+test_disable_saved_count=1
. ./test-lib.sh
if ! test_have_prereq PERL; then
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..374cdb2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -522,11 +522,19 @@ test_skip () {
esac
}
+test_emit_plan () {
+ if [ -z "$test_plan_emitted" -a -n "$test_count_saved" ]; then
+ say "1..$test_count_saved"
+ test_plan_emitted=y
+ fi
+}
+
test_expect_failure () {
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
export test_prereq
+ test_emit_plan
if ! test_skip "$@"
then
say >&3 "checking known breakage: $2"
@@ -545,6 +553,7 @@ test_expect_success () {
test "$#" = 2 ||
error "bug in the test script: not 2 or 3 parameters to test-expect-success"
export test_prereq
+ test_emit_plan
if ! test_skip "$@"
then
say >&3 "expecting success: $2"
@@ -860,12 +869,14 @@ test_done () {
fi
case "$test_failure" in
0)
+ [ -z "$skip_all" ] && echo "$test_count" > "$test_count_file"
+
# Maybe print SKIP message
[ -z "$skip_all" ] || skip_all=" # SKIP $skip_all"
if test $test_external_has_tap -eq 0; then
say_color pass "# passed all $msg"
- say "1..$test_count$skip_all"
+ [ -z "$test_plan_emitted" ] && say "1..$test_count$skip_all"
fi
test -d "$remove_trash" &&
@@ -877,7 +888,7 @@ test_done () {
*)
if test $test_external_has_tap -eq 0; then
say_color error "# failed $test_failure among $msg"
- say "1..$test_count"
+ [ -z "$test_plan_emitted" ] && say "1..$test_count"
fi
exit 1 ;;
@@ -896,6 +907,18 @@ then
fi
GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+mkdir -p "$TEST_DIRECTORY"/test-counts
+test_count_file="$TEST_DIRECTORY"/test-counts/$(basename "$0" .sh)
+test_count_saved=$(
+ if [ -n "$test_disable_saved_count" ]; then
+ :
+ # the saved count is only valid if the file is newer than the test
+ elif [ -f "$test_count_file" -a "$test_count_file" -nt "$0" ]; then
+ cat "$test_count_file" 2>/dev/null
+ fi
+ # otherwise we leave the variable empty
+)
+
if test -n "$valgrind"
then
make_symlink () {
--
1.7.7.rc0.420.g468b7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] test-lib: save test counts across invocations
2011-09-01 13:08 [PATCH] test-lib: save test counts across invocations Thomas Rast
@ 2011-09-01 16:14 ` Junio C Hamano
2011-09-01 16:38 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2011-09-01 16:14 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jeff King
Thomas Rast <trast@student.ethz.ch> writes:
> Save the number of tests run ($test_count) in a file under
> test-counts/. Then when sourcing test-lib.sh the next time, compare
> the timestamps.
... which is this logic ...
> +test_count_file="$TEST_DIRECTORY"/test-counts/$(basename "$0" .sh)
> +test_count_saved=$(
> + if [ -n "$test_disable_saved_count" ]; then
> + :
> + # the saved count is only valid if the file is newer than the test
> + elif [ -f "$test_count_file" -a "$test_count_file" -nt "$0" ]; then
> + cat "$test_count_file" 2>/dev/null
> + fi
> + # otherwise we leave the variable empty
> +)
I think the patch is cute, but I however do not think this is sufficient
to catch prerequisite changes, unfortunately. I'd rather leave the total
unknown than giving incorrect numbers.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test-lib: save test counts across invocations
2011-09-01 13:08 [PATCH] test-lib: save test counts across invocations Thomas Rast
2011-09-01 16:14 ` Junio C Hamano
@ 2011-09-01 16:38 ` Jeff King
2011-09-01 18:38 ` Alex Vandiver
2011-09-02 12:39 ` Thomas Rast
1 sibling, 2 replies; 6+ messages in thread
From: Jeff King @ 2011-09-01 16:38 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
On Thu, Sep 01, 2011 at 03:08:45PM +0200, Thomas Rast wrote:
> Save the number of tests run ($test_count) in a file under
> test-counts/. Then when sourcing test-lib.sh the next time, compare
> the timestamps. If the counts file is older than the test, discard.
> Otherwise use the count that we saved and give prove the test plan
> ("1..N") up front.
Hmm. What happens when we're wrong? Does our eye-candy just print
something non-sensical like "13/12", or does prove actually care that we
run the right number of tests?
> Sparked by a discussion on G+. I think this is the "simple" approach.
> The "cute" approach would be to let test-lib.sh define test_* as
> test-counting dummies once, source the test script itself (avoiding
> the sourcing loop with test-lib) to count what it does, then do the
> real work.
I don't think the "cute" approach will ever be accurate. Deciding
whether to run later tests sometimes depends on the results of earlier
tests, in at least two cases:
1. Some tests find out which capabilities the system has, and set
prerequisites. You need to actually run those tests, not make them
counting dummies.
2. Some tests create state that we then iterate on. For example, I
think the mailinfo tests do something like:
test_expect_success 'split' '
git mailsplit -o patches mbox
'
for i in patches/*; do
test_expect_success "check patch $i" '
git mailinfo $i >output
...
'
done
You'd get an inaccurate count if you didn't actually run the
mailsplit command.
Anyway, this whole thing is a cute idea, and I do love eye candy, but I
wonder if it's worth the complexity. All this is telling us is how far
into each of the scripts it is. But we have literally hundreds of test
scripts, all with varying numbers of tests of varying speeds, and you're
probably running 16 or more at one time. So it doesn't tell you what you
really want to know, which is: how soon will the test suite probably be
done running.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] test-lib: save test counts across invocations
2011-09-01 16:38 ` Jeff King
@ 2011-09-01 18:38 ` Alex Vandiver
2011-09-01 18:45 ` Jeff King
2011-09-02 12:39 ` Thomas Rast
1 sibling, 1 reply; 6+ messages in thread
From: Alex Vandiver @ 2011-09-01 18:38 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, git, Junio C Hamano
On Thu, 2011-09-01 at 12:38 -0400, Jeff King wrote:
> Hmm. What happens when we're wrong? Does our eye-candy just print
> something non-sensical like "13/12", or does prove actually care that we
> run the right number of tests?
prove very much does care -- having a mismatch between the number of
tests planned and the number of tests run is an error in the testfile,
and is reported as such in big red text. This is because stating how
many tests you plan to run gives prove a way (in addition to the exit
status) to know if the test stopped prematurely, so all mismatches
between plan and actual test counts are reported as testfile failures.
As far as I know prove doesn't have a way to print the estimated time
remaining, though using the contents of the .prove file (if you ran
prove --state=save) to guess it wouldn't be all that hard of a change.
- Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test-lib: save test counts across invocations
2011-09-01 18:38 ` Alex Vandiver
@ 2011-09-01 18:45 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2011-09-01 18:45 UTC (permalink / raw)
To: Alex Vandiver; +Cc: Thomas Rast, git, Junio C Hamano
On Thu, Sep 01, 2011 at 02:38:54PM -0400, Alex Vandiver wrote:
> On Thu, 2011-09-01 at 12:38 -0400, Jeff King wrote:
> > Hmm. What happens when we're wrong? Does our eye-candy just print
> > something non-sensical like "13/12", or does prove actually care that we
> > run the right number of tests?
>
> prove very much does care -- having a mismatch between the number of
> tests planned and the number of tests run is an error in the testfile,
> and is reported as such in big red text. This is because stating how
> many tests you plan to run gives prove a way (in addition to the exit
> status) to know if the test stopped prematurely, so all mismatches
> between plan and actual test counts are reported as testfile failures.
Thanks. I suspected something like that, but was too lazy to look. :)
Given that our methods for automatically determining the number of tests
are so flaky, and that prove will treat it so seriously, it doesn't seem
worth pursuing to me.
We already handle the premature abort case by trapping exit from the
shell before the script calls test_done. So I don't think that is a
feature of prove that we particularly care about.
> As far as I know prove doesn't have a way to print the estimated time
> remaining, though using the contents of the .prove file (if you ran
> prove --state=save) to guess it wouldn't be all that hard of a change.
That would be a neat feature. In practice, I know about how many tests
there are total (~7500), and how long it takes to run on my system (~60
seconds), so I can do the math myself. Still, a little more eye candy
couldn't hurt. ;)
If I underestand the code correctly, we could even write our own custom
"formatter" for git and use it via "prove --formatter".
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] test-lib: save test counts across invocations
2011-09-01 16:38 ` Jeff King
2011-09-01 18:38 ` Alex Vandiver
@ 2011-09-02 12:39 ` Thomas Rast
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Rast @ 2011-09-02 12:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Jeff King wrote:
> Anyway, this whole thing is a cute idea, and I do love eye candy, but I
> wonder if it's worth the complexity. All this is telling us is how far
> into each of the scripts it is. But we have literally hundreds of test
> scripts, all with varying numbers of tests of varying speeds, and you're
> probably running 16 or more at one time. So it doesn't tell you what you
> really want to know, which is: how soon will the test suite probably be
> done running.
I guess you're right. Let's drop this, then.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-02 12:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-01 13:08 [PATCH] test-lib: save test counts across invocations Thomas Rast
2011-09-01 16:14 ` Junio C Hamano
2011-09-01 16:38 ` Jeff King
2011-09-01 18:38 ` Alex Vandiver
2011-09-01 18:45 ` Jeff King
2011-09-02 12:39 ` Thomas Rast
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).