ocket8888 commented on a change in pull request #3033: Compare fixup
URL: https://github.com/apache/trafficcontrol/pull/3033#discussion_r234320159
##########
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:
If it's not a string, then it's not the field I'm looking for and that would
mean I'm deleting an object that _should_ be included in the comparison. Doing
this generically just isn't possible, unfortunately, since at its core this is
a hard-coded handling of specific keys for specific endpoints.
The other possibility I looked at was doing if statements looking at the
actual route name, but ultimately I decided that this would be more
forward-compatible with changes to the names of routes and introduction of new
routes.
What I definitely can (and should) do is move the logic for doing this
modification to a JSON object into its own `func` and just call that on each of
the responses.
----------------------------------------------------------------
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
