Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/encoding/readable.sh fix #6714

Merged
1 commit merged into from Nov 26, 2015
Merged

Conversation

aiicore
Copy link
Contributor

@aiicore aiicore commented Nov 26, 2015

  1. Fix formatting (mixed tabs and spaces)
  2. Status of skipped types was showing always last version in directory

Signed-off-by: Igor Podoski igor.podoski@ts.fujitsu.com

1. Fix formatting (mixed tabs and spaces)
2. Status of skipped types was showing always last version in directory

Signed-off-by: Igor Podoski <igor.podoski@ts.fujitsu.com>

if [ "$iv" = "$version" ]; then
break
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see $version in this script, is it taken from environment?

I would suggest to use 'set -u' just like 'set -e' (in other bash scripts too) to prevent running with undefined variables inside.

@ghost
Copy link

ghost commented Nov 26, 2015

could you please separate the cosmetic part from the functional part ? It is a sensitive test and it will help be 100% sure the review does not miss anything :-)

@ghost ghost added cleanup core tests and removed core labels Nov 26, 2015
fi
done
if [ -n "$incompat" ]; then
echo "skipping incompat $type version $arversion, changed at $incompat < code $myversion"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dachary , are u sure? From the functional part I changed only:
echo "skipping incompat $type version $arversion, changed at $iv < code $myversion"
to
echo "skipping incompat $type version $arversion, changed at $incompat < code $myversion"

And If I split this into two, this line had indent change, so secondary commit will be looking just like this one.

@ghost
Copy link

ghost commented Nov 26, 2015

I manually verified that each change below is only white space. With the exception of s/$iv/$incompat/ in the echo string.

git --no-pager diff --ignore-space-change master..refs/pull/6714/merge src/test/encoding/readable.sh
diff --git a/src/test/encoding/readable.sh b/src/test/encoding/readable.sh
index f387bd1..f1cc7f4 100755
--- a/src/test/encoding/readable.sh
+++ b/src/test/encoding/readable.sh
@@ -12,43 +12,43 @@ numtests=0
 
 myversion=`./ceph-dencoder version`
 
-for arversion in `ls -v $dir/archive`
-do
+for arversion in `ls -v $dir/archive`; do
   vdir="$dir/archive/$arversion"
-#    echo $vdir
+  #echo $vdir
 
   if [ ! -d "$vdir/objects" ]; then
     continue;
   fi
 
-    for type in `ls $vdir/objects`
-    do
+  for type in `ls $vdir/objects`; do
     if ./ceph-dencoder type $type 2>/dev/null; then
-#      echo "type $type";
+      #echo "type $type";
       echo "        $vdir/objects/$type"
 
       # is there a fwd incompat change between $arversion and $version?
       incompat=""
       sawarversion=0
-       for iv in `ls -v $dir/archive`
-       do
+      for iv in `ls -v $dir/archive`; do
         if [ "$iv" = "$arversion" ]; then
           sawarversion=1
         fi
+
         if [ $sawarversion -eq 1 ] && [ -e "$dir/archive/$iv/forward_incompat/$type" ]; then
           incompat="$iv"
         fi
+
         if [ "$iv" = "$version" ]; then
           break
         fi
       done
+
       if [ -n "$incompat" ]; then
-       echo "skipping incompat $type version $arversion, changed at $iv < code $myversion"
+        echo "skipping incompat $type version $arversion, changed at $incompat < code $myversion"
         continue
       fi
 
       for f in `ls $vdir/objects/$type`; do
-#      echo "\t$vdir/$type/$f"
+        #echo "\t$vdir/$type/$f"
         if ! ./ceph-dencoder type $type import $vdir/objects/$type/$f decode dump_json > $tmp1; then
           echo "**** failed to decode $vdir/objects/$type/$f ****"
           failed=$(($failed + 1))
@@ -64,8 +64,7 @@ do
         # nondeterministically.  compare the sorted json
         # output.  this is a weaker test, but is better than
         # nothing.
-       if ! ./ceph-dencoder type $type is_deterministic
-       then
+        if ! ./ceph-dencoder type $type is_deterministic; then
           echo "  sorting json output for nondeterministic object"
           for f in $tmp1 $tmp2; do
             sort $f | sed 's/,$//' > $f.new

@ghost ghost self-assigned this Nov 26, 2015
ghost pushed a commit that referenced this pull request Nov 26, 2015
test/encoding/readable.sh fix

Reviewed-by: Loic Dachary <ldachary@redhat.com>
@ghost ghost merged commit c4fff6f into ceph:master Nov 26, 2015
@ghost
Copy link

ghost commented Nov 26, 2015

@aiicore in the future, could you make it so cosmetic cleanups are accompanied by a functional change to balance the time spent reviewing them ( https://github.com/ceph/ceph/blob/master/CONTRIBUTING.rst ) ?

@aiicore
Copy link
Contributor Author

aiicore commented Nov 26, 2015

@dachary Ok, sure, thanks!

@aiicore aiicore deleted the test_encoding_readable branch November 26, 2015 14:19
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants