Cass graphite intg test #67

geewarges · 2017-05-08T12:00:19Z

pair programmed with @Jozo95 results
-add a new dbconnector integration test file.
-updated integration file so that it runs integration test from cdr to graphite "all the way up".

johan-bjareholt · 2017-05-08T12:05:13Z

test/integration.sh

@@ -51,21 +51,36 @@ docker exec -it "$container_name" cqlsh -f "$schema_file"

 # It's up!
 # Run integration test
-echo "Running integration test"
-int_test=$(./cdr_cass_integration_test.sh "$container_name" 2>&1)


Why did you change this?
On the lines above we create and start a cassandra container on port 9043 so we can run the tests without having to manually start cassandra as well as not having to shut down cassandra if it is already running on your local machine. This breaks it.

Lilja · 2017-05-08T12:25:56Z

test/cdr_cass_integration_test.sh

@@ -59,7 +59,9 @@ grep -q "${cassandra_port}=\"${cassandra_it_port}\"" "$app_conf_path" && echo -n

 # Finally, run the jar.
 # Run with timeout
-timeout $timeout java -Dnetty.epoll.enabled=false -Dconfig.file=$(basename "$app_conf_path") -jar "$jar_directory"
+pwd


Lilja · 2017-05-08T12:26:48Z

test/cdr_cass_integration_test.sh

@@ -76,7 +78,7 @@ docker exec -i "$cass_container_name" cqlsh << EOF > "$temp_cassandra_result_fil
 use $cassandra_keyspace;
 select count(*) from $cassandra_cdr_table_name;
 EOF
-
+#$cassandra_cdr_table_name;


What does this even do?

Lilja · 2017-05-08T12:29:32Z

test/dbcIntegrationTest.sh

+# back to the directory you were in at start
+popd
+
+#now run curl to check if the data is at graphite. in this case we are lookng att callplan


Grammar/Spelling
Please use quotes for referencing speficic things here. we are looking for the product "CallPlanNormal"

Lilja · 2017-05-08T12:30:00Z

test/dbcIntegrationTest.sh

+
+#check if the file contain data by
+# Read from file and grep for rows.
+#echo ${grep -Fxq "\[\]" $tempGraphiteFile}


Remove commented code

Lilja · 2017-05-08T12:30:31Z

test/dbcIntegrationTest.sh

+fi
+
+#now remove the temp file 
+rm -f $tempGraphiteFile


Why -f? this can be abused. Remove -f.

Lilja · 2017-05-08T12:31:01Z

test/integration.sh

@@ -51,21 +51,36 @@ docker exec -it "$container_name" cqlsh -f "$schema_file"

 # It's up!
 # Run integration test
-echo "Running integration test"
-int_test=$(./cdr_cass_integration_test.sh "$container_name" 2>&1)


Lilja · 2017-05-08T12:31:47Z

test/cdr_cass_integration_test.sh

@@ -12,7 +12,7 @@ app_conf_path="src/main/resources/application.conf"
 batch_limit="batch\.limit"
 batch_size_limit="cassandra\.element\.batch\.size"
 cassandra_port="port"
-cassandra_it_port=9043
+cassandra_it_port=9042


Please use 9043.

Lilja · 2017-05-08T12:34:08Z

test/dbcIntegrationTest.sh

+#now run curl to check if the data is at graphite. in this case we are lookng att callplan
+curl '127.0.0.1:2000/render?target=qvantel.product.voice.CallPlanNormal&format=json' -o $tempGraphiteFile
+
+#check if the file contain data by


Inconsistent comment whitespace. The next comment uses
# Comment and this one uses #Comment. Use the first one.

Lilja · 2017-05-08T12:34:33Z

test/dbcIntegrationTest.sh

+if grep -Fxq "[]" $tempGraphiteFile
+then
+#there are no records in this file
+echo "the dbconnector integration test failed!"


Indentation

Lilja

See my earlier review

flygare · 2017-05-08T12:33:29Z

test/cdr_cass_integration_test.sh

@@ -59,7 +59,9 @@ grep -q "${cassandra_port}=\"${cassandra_it_port}\"" "$app_conf_path" && echo -n

 # Finally, run the jar.
 # Run with timeout
-timeout $timeout java -Dnetty.epoll.enabled=false -Dconfig.file=$(basename "$app_conf_path") -jar "$jar_directory"
+pwd


Why is this here?

flygare · 2017-05-08T12:34:25Z

test/cdr_cass_integration_test.sh

@@ -76,7 +78,7 @@ docker exec -i "$cass_container_name" cqlsh << EOF > "$temp_cassandra_result_fil
 use $cassandra_keyspace;
 select count(*) from $cassandra_cdr_table_name;
 EOF
-
+#$cassandra_cdr_table_name;


This comment provides nothing

johan-bjareholt · 2017-05-10T08:40:21Z

test/cdr_cass_integration_test.sh

@@ -85,18 +84,18 @@ has_records=$(cat "$temp_cassandra_result_file" | grep -oP '\s+[0-9]+')

 # use head and tail to get the first and last.
 has_records_cdr=$(echo "$has_records" | head -n+1)
-
+#


johan-bjareholt · 2017-05-10T08:41:07Z

test/cdr_cass_integration_test.sh

 # cleanup
 if [ -f "$temp_cassandra_result_file" ]
 then
-    echo "Removed temp file"


Why switch all 4 space identations to 3 space identations?

johan-bjareholt · 2017-05-10T08:42:41Z

test/dbcIntegrationTest.sh

+#!/bin/bash
+
+app_conf_path="src/main/resources/application.conf"
+limit="theLimit"


Please rename

johan-bjareholt · 2017-05-10T08:43:04Z

test/dbcIntegrationTest.sh

+popd
+
+#now run curl to check if the data is at graphite. in this case we are looking for "callplanNormal"
+curl '127.0.0.1:2090/render?target=qvantel.product.voice.CallPlanNormal&format=json' -o $tempGraphiteFile


Please set variables for IP and port

johan-bjareholt · 2017-05-10T08:43:33Z

test/dbcIntegrationTest.sh

+if grep -Fxq "[]" $tempGraphiteFile
+then
+#there are no records in this file
+echo "the dbconnector integration test failed!"


Add identation

johan-bjareholt · 2017-05-10T08:44:40Z

test/dbcIntegrationTest.sh

+then
+#there are no records in this file
+echo "the dbconnector integration test failed!"
+echo "[-failed-]"


You need to set a non-zero exit code upon fail, otherwise travis will not report this as a failed test.
If you do cleanup in the end of this file you most likely want to set a variable with the exit code and to "exit $myvar" in the end

Saw that you later in integration.sh use grep on this text.
Sure it works but it is very bad and can easily break, so please fix anyway.

johan-bjareholt · 2017-05-10T08:55:30Z

test/integration.sh


 # Finally, report back
 if [ -n "$(echo $int_test | grep '[SUCCESS]')" ] && [ $code -eq 0 ]
 then
-    echo "Integration test successful"
-    exit 0


Why remove exit 0?

geewarges added 6 commits April 19, 2017 13:23

add dbconnector integration test

5e8c8b7

add dbconnector integration test

bbd3837

add new dbc Integration test file

039f50e

fixet integration dbconnector integration test

0c3243a

deleted unnecessary files

d35a97a

fixed an if statment for showing results of test

c874d3d

johan-bjareholt requested changes May 8, 2017

View reviewed changes

Lilja approved these changes May 8, 2017

View reviewed changes

Lilja requested changes May 8, 2017

View reviewed changes

flygare reviewed May 8, 2017

View reviewed changes

geewarges added 4 commits May 9, 2017 17:17

fixet create new test container

5a8b96e

deleted unnecessary file

257bd30

cleaned and fixet comments and grammar errors

5315795

fixed some grammar error

4bd636d

johan-bjareholt requested changes May 10, 2017

View reviewed changes

changed limit variable

ea740b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cass graphite intg test #67

Cass graphite intg test #67

geewarges commented May 8, 2017

johan-bjareholt May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja May 8, 2017

Lilja left a comment

flygare May 8, 2017

flygare May 8, 2017

johan-bjareholt May 10, 2017

johan-bjareholt May 10, 2017

johan-bjareholt May 10, 2017 •

edited

Loading

johan-bjareholt May 10, 2017

johan-bjareholt May 10, 2017

johan-bjareholt May 10, 2017

johan-bjareholt May 10, 2017

geewarges May 10, 2017

johan-bjareholt May 10, 2017

Cass graphite intg test #67

Are you sure you want to change the base?

Cass graphite intg test #67

Conversation

geewarges commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lilja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johan-bjareholt May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johan-bjareholt May 10, 2017 •

edited

Loading