[PATCH] test: aggregate-results.sh: consistent style. zero forks.

classic Classic list List threaded Threaded
5 messages Options
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

[PATCH] test: aggregate-results.sh: consistent style. zero forks.

- all variables in $((...)) without leading $
- all comparisons use -gt, -eq or -ne
- no -a nor -o inside [ ... ] expressions
- all indentation levels using one tab

Dropped unnecessary empty string check when reading results files.

Replaced pluralize() which was executed in subshell with
pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
$1. Calls to pluralize_s are done in context of current shell, so
no forks to subshells executed.
---

I have some other changes planned for this -- before that I wanted
to clead some accumulated inconsistencies...

2 changes I wanted but did not do:
   1) #!/bin/sh
   2) set -euf

 test/aggregate-results.sh | 92 +++++++++++++++------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)

diff --git a/test/aggregate-results.sh b/test/aggregate-results.sh
index 63228546..0f94948b 100755
--- a/test/aggregate-results.sh
+++ b/test/aggregate-results.sh
@@ -13,81 +13,61 @@ do
  while read type value
  do
  case $type in
- '')
- continue ;;
  fixed)
- fixed=$(($fixed + $value)) ;;
+ fixed=$((fixed + value)) ;;
  success)
- success=$(($success + $value)) ;;
+ success=$((success + value)) ;;
  failed)
- failed=$(($failed + $value)) ;;
+ failed=$((failed + value)) ;;
  broken)
- broken=$(($broken + $value)) ;;
+ broken=$((broken + value)) ;;
  total)
- total=$(($total + $value)) ;;
+ total=$((total + value)) ;;
  esac
  done <"$file"
 done
 
-pluralize () {
-    case $2 in
- 1)
-    case $1 in
- test)
-    echo test ;;
- failure)
-    echo failure ;;
-    esac
-    ;;
- *)
-    case $1 in
- test)
-    echo tests ;;
- failure)
-    echo failures ;;
-    esac
-    ;;
-    esac
-}
+pluralize_s () { [ $1 -eq 1 ] && s='' || s='s'; }
 
 echo "Notmuch test suite complete."
-if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
-    tests=$(pluralize "test" $total)
-    printf "All $total $tests "
-    if [ "$broken" = "0" ]; then
- echo "passed."
-    else
- failures=$(pluralize "failure" $broken)
- echo "behaved as expected ($broken expected $failures)."
-    fi;
+
+if [ $fixed -eq 0 ] && [ $failed -eq 0 ]; then
+ pluralize_s $total
+ printf "All $total test$s "
+ if [ $broken -eq 0 ]; then
+ echo "passed."
+ else
+ pluralize_s $broken
+ echo "behaved as expected ($broken expected failure$s)."
+ fi
 else
-    echo "$success/$total tests passed."
-    if [ "$broken" != "0" ]; then
- tests=$(pluralize "test" $broken)
- echo "$broken broken $tests failed as expected."
-    fi
-    if [ "$fixed" != "0" ]; then
- tests=$(pluralize "test" $fixed)
- echo "$fixed broken $tests now fixed."
-    fi
-    if [ "$failed" != "0" ]; then
- tests=$(pluralize "test" $failed)
- echo "$failed $tests failed."
-    fi
+ echo "$success/$total tests passed."
+ if [ $broken -ne 0 ]; then
+ pluralize_s $broken
+ echo "$broken broken test$s failed as expected."
+ fi
+ if [ $fixed -ne 0 ]; then
+ pluralize_s $fixed
+ echo "$fixed broken test$s now fixed."
+ fi
+ if [ $failed -ne 0 ]; then
+ pluralize_s $failed
+ echo "$failed test$s failed."
+ fi
 fi
 
-skipped=$(($total - $fixed - $success - $failed - $broken))
-if [ "$skipped" != "0" ]; then
-    tests=$(pluralize "test" $skipped)
-    echo "$skipped $tests skipped."
+skipped=$((total - fixed - success - failed - broken))
+if [ $skipped -ne 0 ]; then
+ pluralize_s $skipped
+ echo "$skipped test$s skipped."
 fi
 
 # Note that we currently do not consider skipped tests as failing the
 # build.
 
-if [ $success -gt 0 -a $fixed -eq 0 -a $failed -eq 0 ]
+if [ $success -gt 0 ] && [ $fixed -eq 0 ] && [ $failed -eq 0 ]
 then
-    exit 0
+ exit 0
 else
-    exit 1
+ exit 1
 fi
--
2.21.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] test: aggregate-results.sh: consistent style. zero forks.

On Tue 2019-06-04 22:46:24 +0300, Tomi Ollila wrote:

> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>
> Dropped unnecessary empty string check when reading results files.
>
> Replaced pluralize() which was executed in subshell with
> pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
> $1. Calls to pluralize_s are done in context of current shell, so
> no forks to subshells executed.
I'm fine with all these changes, but:

> -if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
> +if [ $fixed -eq 0 ] && [ $failed -eq 0 ]; then

I think we've set $fixed and $failed above to a non-empty string value,
so this is technically correct.  But the shell programmer nit-picker in
me gets nervous seeing any variable used inside a test without proper
wrapping, and i'm going to have trouble adopting $foo instead of "$foo"
in other shell scripts.  it seems to require a lot of global reasoning
about the state of a given variable to use it without quotes safely, and
it introduces some subtle requirements (like, no unsetting these
variables and no setting them to the empty string).

So anyway, i don't see the harm in using "$foo" instead in this case,
and it seems like it actually reduces the cognitive burden of
maintiaining the code.  i'm happy to listen to any compelling story
yo've got for why this is an important change, but i'd prefer to avoid
it.  (i don't at all mind unwrapping "0" to 0 though)

thanks for doing this cleanup!

     --dkg

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (233 bytes) Download Attachment
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

[PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

- all variables in $((...)) without leading $
- all comparisons use -gt, -eq or -ne
- no -a nor -o inside [ ... ] expressions
- all indentation levels using one tab

Dropped unnecessary empty string check when reading results files.

Replaced pluralize() which was executed in subshell with
pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
$1. Calls to pluralize_s are done in context of current shell, so
no forks to subshells executed.
---

V2: added quotes all "$variable" references where empty values or
    IFS characters could make a difference. Not in this script, but
    servers better example as a usage style elsewhere (where it could
    matter).

 test/aggregate-results.sh | 92 +++++++++++++++------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)

diff --git a/test/aggregate-results.sh b/test/aggregate-results.sh
index 63228546..05fb0a92 100755
--- a/test/aggregate-results.sh
+++ b/test/aggregate-results.sh
@@ -13,81 +13,61 @@ do
  while read type value
  do
  case $type in
- '')
- continue ;;
  fixed)
- fixed=$(($fixed + $value)) ;;
+ fixed=$((fixed + value)) ;;
  success)
- success=$(($success + $value)) ;;
+ success=$((success + value)) ;;
  failed)
- failed=$(($failed + $value)) ;;
+ failed=$((failed + value)) ;;
  broken)
- broken=$(($broken + $value)) ;;
+ broken=$((broken + value)) ;;
  total)
- total=$(($total + $value)) ;;
+ total=$((total + value)) ;;
  esac
  done <"$file"
 done
 
-pluralize () {
-    case $2 in
- 1)
-    case $1 in
- test)
-    echo test ;;
- failure)
-    echo failure ;;
-    esac
-    ;;
- *)
-    case $1 in
- test)
-    echo tests ;;
- failure)
-    echo failures ;;
-    esac
-    ;;
-    esac
-}
+pluralize_s () { [ "$1" -eq 1 ] && s='' || s='s'; }
 
 echo "Notmuch test suite complete."
-if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
-    tests=$(pluralize "test" $total)
-    printf "All $total $tests "
-    if [ "$broken" = "0" ]; then
- echo "passed."
-    else
- failures=$(pluralize "failure" $broken)
- echo "behaved as expected ($broken expected $failures)."
-    fi;
+
+if [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]; then
+ pluralize_s "$total"
+ printf "All $total test$s "
+ if [ "$broken" -eq 0 ]; then
+ echo "passed."
+ else
+ pluralize_s "$broken"
+ echo "behaved as expected ($broken expected failure$s)."
+ fi
 else
-    echo "$success/$total tests passed."
-    if [ "$broken" != "0" ]; then
- tests=$(pluralize "test" $broken)
- echo "$broken broken $tests failed as expected."
-    fi
-    if [ "$fixed" != "0" ]; then
- tests=$(pluralize "test" $fixed)
- echo "$fixed broken $tests now fixed."
-    fi
-    if [ "$failed" != "0" ]; then
- tests=$(pluralize "test" $failed)
- echo "$failed $tests failed."
-    fi
+ echo "$success/$total tests passed."
+ if [ "$broken" -ne 0 ]; then
+ pluralize_s "$broken"
+ echo "$broken broken test$s failed as expected."
+ fi
+ if [ "$fixed" -ne 0 ]; then
+ pluralize_s "$fixed"
+ echo "$fixed broken test$s now fixed."
+ fi
+ if [ "$failed" -ne 0 ]; then
+ pluralize_s "$failed"
+ echo "$failed test$s failed."
+ fi
 fi
 
-skipped=$(($total - $fixed - $success - $failed - $broken))
-if [ "$skipped" != "0" ]; then
-    tests=$(pluralize "test" $skipped)
-    echo "$skipped $tests skipped."
+skipped=$((total - fixed - success - failed - broken))
+if [ "$skipped" -ne 0 ]; then
+ pluralize_s "$skipped"
+ echo "$skipped test$s skipped."
 fi
 
 # Note that we currently do not consider skipped tests as failing the
 # build.
 
-if [ $success -gt 0 -a $fixed -eq 0 -a $failed -eq 0 ]
+if [ "$success" -gt 0 ] && [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]
 then
-    exit 0
+ exit 0
 else
-    exit 1
+ exit 1
 fi
--
2.21.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

On Mon 2019-06-10 21:39:23 +0300, Tomi Ollila wrote:

> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>
> Dropped unnecessary empty string check when reading results files.
>
> Replaced pluralize() which was executed in subshell with
> pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
> $1. Calls to pluralize_s are done in context of current shell, so
> no forks to subshells executed.
> ---
>
> V2: added quotes all "$variable" references where empty values or
>     IFS characters could make a difference. Not in this script, but
>     servers better example as a usage style elsewhere (where it could
>     matter).
LGTM.  Thanks, Tomi!

       --dkg

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (233 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

In reply to this post by Tomi Ollila-2
Tomi Ollila <[hidden email]> writes:

> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>

pushed to master

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch