[GitHub] dangogh commented on a change in pull request #3033: Compare fixup

[GitBox <gi...@apache.org>]

dangogh commented on a change in pull request #3033: Compare fixup
URL: https://github.com/apache/trafficcontrol/pull/3033#discussion_r234297178
 
 

 ##########
 File path: traffic_ops/testing/compare/compare.go
 ##########
 @@ -157,74 +159,301 @@ func testRoute(tos []*Connect, route string) {
 		log.Fatalf("Error occurred `GET`ting %s from %s: %s\n", route, res[1].TO.URL, res[1].Error.Error())
 	}
 
+	ctypeA, ctypeB := res[0].Res.Header.Get("Content-Type"), res[1].Res.Header.Get("Content-Type")
+	if ctypeA != ctypeB {
+		log.Printf("ERROR: Differing content types for route %s - %s reports %s but %s reports %s !\n",
+		           route, res[0].TO.URL, ctypeA, res[1].TO.URL, ctypeB)
+		return
+	}
+
+	// Handle JSON data - note that this WILL NOT be used for endpoints that report the wrong content-type
+	// (ignores charset encoding)
+	if strings.Contains(ctypeA, "application/json") {
+		handleJSONResponse(&res, route)
+
+	// WARNING: treats ALL non-JSON responses as plaintext - should usually operate as expected, but
+	// optimizations could be made for other structures
+	} else {
+		handlePlainTextResponse(&res, route)
+	}
+}
+
+// Given a slice of (exactly two) result objects, compares the plain text content of their responses
+// and write them to files if they differ. Ignores Traffic Ops headers in the response (to the
+// degree possible)
+func handlePlainTextResponse(responses *[]result, route string) {
+
+	// I avoid using `defer` to close the bodies because I want to do it as quickly as possible
+	result0, err := ioutil.ReadAll((*responses)[0].Res.Body)
+	(*responses)[0].Res.Body.Close()
+	if err != nil {
+		(*responses)[1].Res.Body.Close()
+		log.Fatalf("Failed to read response body from %s/%s: %s\n", (*responses)[0].TO.URL, route, err.Error())
+	}
+
+	result1, err := ioutil.ReadAll((*responses)[1].Res.Body)
+	(*responses)[1].Res.Body.Close()
+	if err != nil {
+		log.Fatalf("Failed to read response body from %s/%s: %s\n", (*responses)[1].TO.URL, route, err.Error())
+	}
+
 	// Check for Traffic Ops headers and remove them before comparison
-	refResult := res[0].Res
-	testResult := res[1].Res
+	result0Str, result1Str := string(result0), string(result1)
+	scrubbedResult0, scrubbedResult1 := "", ""
 	if strings.Contains(route, "configfiles") {
-		refLines := strings.Split(refResult, "\n")
-		testLines := strings.Split(testResult, "\n")
+		lines0 := strings.Split(result0Str, "\n")
+		lines1 := strings.Split(result1Str, "\n")
 
 		// If the two files have different numbers of lines, they definitely differ
-		if len(refLines) != len(testLines) {
-			log.Print("Diffs from ", route, " written to")
-			p, err := res[0].TO.writeResults(route, refResult)
-			if err != nil {
-				log.Fatalf("Error writing results for %s: %s\n", route, err.Error())
-			}
-			log.Print(" ", p)
-			p, err = res[1].TO.writeResults(route, testResult)
-			if err != nil {
-				log.Fatalf("Error writing results for %s: %s\n", route, err.Error())
-			}
-			log.Print(" and ", p)
+		if len(lines0) != len(lines1) {
+			writeAllResults(route, result0Str, (*responses)[0].TO, result1Str, (*responses)[1].TO)
 			return
 		}
 
-
-		refResult = ""
-		testResult = ""
-
-		for _, refLine := range refLines {
-			if len(refLine) < len(SHORT_HEADER) {
-				refResult += refLine
-			} else if refLine[:len(SHORT_HEADER)] != SHORT_HEADER {
-				if len(refLine) >= len(LONG_HEADER) {
-					if refLine[:len(LONG_HEADER)] != LONG_HEADER {
-						refResult += refLine
+		for _, line := range lines0 {
+			if len(line) < len(SHORT_HEADER) {
+				scrubbedResult0 += line
+			} else if line[:len(SHORT_HEADER)] != SHORT_HEADER {
+				if len(line) >= len(LONG_HEADER) {
+					if line[:len(LONG_HEADER)] != LONG_HEADER {
+						scrubbedResult0 += line
 					}
 				} else {
-					refResult += refLine
+					scrubbedResult0 += line
 				}
 			}
 		}
 
-		for _, testLine := range testLines {
-			if len(testLine) < len(SHORT_HEADER) {
-				testResult += testLine
-			} else if testLine[:len(SHORT_HEADER)] != SHORT_HEADER {
-				if len(testLine) >= len(LONG_HEADER) {
-					if testLine[:len(LONG_HEADER)] != LONG_HEADER {
-						testResult += testLine
+		for _, line := range lines1 {
+			if len(line) < len(SHORT_HEADER) {
+				scrubbedResult1 += line
+			} else if line[:len(SHORT_HEADER)] != SHORT_HEADER {
+				if len(line) >= len(LONG_HEADER) {
+					if line[:len(LONG_HEADER)] != LONG_HEADER {
+						scrubbedResult1 += line
 					}
 				} else {
-					testResult += testLine
+					scrubbedResult1 += line
 				}
 			}
 		}
+	} else {
+		scrubbedResult0 = result0Str
+		scrubbedResult1 = result1Str
 	}
 
-	if refResult == testResult {
-		log.Printf("Identical results (%d bytes) from %s", len(res[0].Res), route)
+	if scrubbedResult0 == scrubbedResult1 {
+		log.Printf("Identical results (%d bytes) from %s\n", len(result0), route)
 	} else {
-		log.Print("Diffs from ", route, " written to")
-		for _, r := range res {
-			p, err := r.TO.writeResults(route, r.Res)
-			if err != nil {
-				log.Fatalf("Error writing results for %s: %s", route, err.Error())
+		writeAllResults(route, result0Str, (*responses)[0].TO, result1Str, (*responses)[1].TO)
+	}
+}
+
+// Given a slice of (exactly two) result objects, compares the JSON content of their responses
+// and write them to files if they differ. Ignores timestamps and Traffic Ops hostnames (to the
+// degree possible)
+func handleJSONResponse(responses *[]result, route string) {
+
+	// I avoid using `defer` to close the bodies because I want to do it as quickly as possible
+
+	result0Orig, err := ioutil.ReadAll((*responses)[0].Res.Body)
+	(*responses)[0].Res.Body.Close()
+	if err != nil {
+		(*responses)[1].Res.Body.Close()
+		log.Fatalf("Failed to read response body from %s/%s: %s\n", (*responses)[0].TO.URL, route, err.Error())
+	}
+	var result0 interface{}
+	if err = json.Unmarshal([]byte(result0Orig), &result0); err != nil {
+		log.Fatalf("Failed to parse response body from %s/%s as JSON: %s\n", (*responses)[0].TO.URL, route, err.Error())
+	}
+
+	result1Orig, err := ioutil.ReadAll((*responses)[1].Res.Body)
+	(*responses)[1].Res.Body.Close()
+	if err != nil {
+		log.Fatalf("Failed to read response body from %s/%s: %s\n", (*responses)[1].TO.URL, route, err.Error())
+	}
+	var result1 interface{}
+	if err = json.Unmarshal(result1Orig, &result1); err != nil {
+		log.Fatalf("Failed to parse response body from %s/%s as JSON: %s\n", (*responses)[1].TO.URL, route, err.Error())
+	}
+
+	// type-safety cast to maps
+	m0, m1 := result0.(map[string]interface{}), result1.(map[string]interface{})
 
 Review comment:
   what happens if the cast fails?   wouldn't it be better to unmarshal directly to the map?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services