dangogh commented on a change in pull request #3033: Compare fixup
URL: https://github.com/apache/trafficcontrol/pull/3033#discussion_r234294017
##########
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{})
+
+ // ... Now we have to iterate over every key in each map to determine
if it should be removed
+ for key, value := range m0 {
+
+ // handles snapshot/new timestamps/hostnames/versions
+ if (key == "response") {
+ switch Type := value.(type) {
+ case map[string]interface{}:
+ if k, in :=
value.(map[string]interface{})["stats"]; in {
+ switch t := k.(type) {
+ case
map[string]interface{}:
+ if val, ok :=
k.(map[string]interface{})["date"]; ok {
+ switch
T := val.(type) {
+
case float64:
+
delete(result0.(map[string]interface{})["response"].(map[string]interface{})["stats"].(map[string]interface{}),
"date")
+
default:
+
log.Printf("found 'stats.date' object - wasn't an integer - %v", T)
Review comment:
yikes! looks like this wasn't run thru `go fmt` for one thing.. and
this nesting level is way too deep to be maintainable. Also a lot of
repetition.. Can we modularize this a bit into funcs?
Also, is checking the type necessary before deleting the entry? Thinking
it's not worth it given the complexity it creates.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
