Re: git commit: TS-1755: add basic logstats tests

[Igor Galić <i....@brainsware.org>]

----- Original Message -----
> Updated Branches:
>   refs/heads/master bb275c046 -> 664ca3987
> 
> 
> TS-1755: add basic logstats tests
> 
[snip]
> 
> ----------------------------------------------------------------------
>  CHANGES                           |    3 +-
>  lib/ts/Layout.cc                  |    4 +-
>  proxy/Makefile.am                 |    4 +
>  proxy/tests/logstats.blog         |  Bin 0 -> 14976 bytes
>  proxy/tests/logstats.json         |  243 ++++++++++++++++++++++++++++++++
>  proxy/tests/logstats.summary      |  164 +++++++++++++++++++++
>  proxy/tests/test_logstats_json    |   26 ++++
>  proxy/tests/test_logstats_summary |   25 ++++
>  8 files changed, 466 insertions(+), 3 deletions(-)
> ----------------------------------------------------------------------
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/664ca398/proxy/tests/test_logstats_json
> ----------------------------------------------------------------------
> diff --git a/proxy/tests/test_logstats_json
> b/proxy/tests/test_logstats_json
> new file mode 100755
> index 0000000..58de9f7
> --- /dev/null
> +++ b/proxy/tests/test_logstats_json
> @@ -0,0 +1,26 @@
> +#! /usr/bin/env bash

This is bash. But..
[snip]
> +
> +set -e # exit on error
> +
> +TMPFILE=`mktemp -t logstats.XXXXXX` || exit 1

You're using ``, which is frowned upon sh syntax. (makes for poor readability
according to Bash gods. Use $( command )

Also: set -e not catches a faulty return from mktemp, no use to use || exit 1.

Please only use UPPERCASE for bash variables, not your OWN.
Is TMPFILE overridable from the outside? No, then THERE'S NO NEED FOR CAPS.

> +
> +# Note that the JSON has a timestamp in it that we have to filter
> out ...
> +./traffic_logstats --log_file "$srcdir/tests/logstats.blog" --json |
> grep -v timestamp > $TMPFILE
> +diff -q $TMPFILE "$srcdir/tests/logstats.json"
> +rm -f $TMPFILE

please use more quotes (around $TMPFILE ), and perhaps use rm -f -- 
so no one can accidentaly pass -r $TMPFILE

> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/664ca398/proxy/tests/test_logstats_summary
> ----------------------------------------------------------------------
> diff --git a/proxy/tests/test_logstats_summary
> b/proxy/tests/test_logstats_summary
> new file mode 100755
> index 0000000..e41a33e
> --- /dev/null
> +++ b/proxy/tests/test_logstats_summary
> @@ -0,0 +1,25 @@
> +#! /usr/bin/env sh

This is "sh". Yay consistency.

> +#  Licensed to the Apache Software Foundation (ASF) under one
> +#  or more contributor license agreements.  See the NOTICE file
> +#  distributed with this work for additional information
> +#  regarding copyright ownership.  The ASF licenses this file
> +#  to you under the Apache License, Version 2.0 (the
> +#  "License"); you may not use this file except in compliance
> +#  with the License.  You may obtain a copy of the License at
> +#
> +#      http://www.apache.org/licenses/LICENSE-2.0
> +#
> +#  Unless required by applicable law or agreed to in writing,
> software
> +#  distributed under the License is distributed on an "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> +#  See the License for the specific language governing permissions
> and
> +#  limitations under the License.
> +
> +set -e # exit on error
> +
> +TMPFILE=`mktemp -t logstats.XXXXXX` || exit 1
> +
> +./traffic_logstats --log_file "$srcdir/tests/logstats.blog"
> --summary > $TMPFILE
> +diff -q $TMPFILE "$srcdir/tests/logstats.summary"
> +rm -f $TMPFILE

same as obove..

In his day job Igor writes fierce reviews of horrific shell scripts,
which is why people have stopped committing stuff altogether..

Igor is NOT paranoid.

-- 
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515  2EA5 4B1D 9E08 A097 C9AE