New issue
Advanced search Search tips

Issue 796658 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

server parses "artifacts" incorrectly as a tree branch instead of field on FullTestLeaf

Project Member Reported by seanmccullough@chromium.org, Dec 20 2017

Issue description

Example log message:
uploadHandler :: {"error":"model: Merge: \"load:news:qq\" expected *AggregateTestLeaf, but got: model.AggregateTest{\"artifacts\":(*model.AggregateTestLeaf)(0xc00da4fce0)}"}

These account for most of the 500s to /testfile/upload for chromium.perf bots.

Quick fix, probably: add extra checks in the full_result json parsing code on the server. That stops the bleeding.

But the existence of all this artisanal (de)serializer code is a bigger problem.

Imagine that https://chromium.googlesource.com/chromium/src/+/master/docs/testing/json_test_results_format.md was a .proto file instead. The test-results server could have handled the new field properly, automatically in the very next build.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/662a047497b2f0ab7f7e4466d19efc533305bcfd

commit 662a047497b2f0ab7f7e4466d19efc533305bcfd
Author: Sean McCullough <seanmccullough@chromium.org>
Date: Thu Dec 21 00:53:34 2017

[test-results] fix parsing for stored broken data saved by buggy parser

test data is pulled from prod datastore based on keys logged in
a /testfile/upload request that returned 500 with the message:

uploadHandler :: {"error":"model: Merge (v) \"http://chromium.github.io/octane/index.html?auto=1\": expected AggregateTest, but got: &model.AggregateTestLeaf{Results:[]model.ResultSummary{model.ResultSummary{Count:1, Type:\"P\"}}, Runtimes:[]model.RuntimeSummary{model.RuntimeSummary{Count:1, Runtime:0}}, Expected:[]string(nil), Bugs:[]string(nil)}"}

Bug:  796658 
Change-Id: Ib5048497eeb97b6986187a0150c12c7eb6885d28
Reviewed-on: https://chromium-review.googlesource.com/838182
Reviewed-by: Tiffany Zhang <zhangtiff@chromium.org>
Commit-Queue: Sean McCullough <seanmccullough@chromium.org>

[add] https://crrev.com/662a047497b2f0ab7f7e4466d19efc533305bcfd/go/src/infra/appengine/test-results/model/testdata/merge-results-small.json
[add] https://crrev.com/662a047497b2f0ab7f7e4466d19efc533305bcfd/go/src/infra/appengine/test-results/model/testdata/merge-results.json
[add] https://crrev.com/662a047497b2f0ab7f7e4466d19efc533305bcfd/go/src/infra/appengine/test-results/model/testdata/merge-full-results.json
[modify] https://crrev.com/662a047497b2f0ab7f7e4466d19efc533305bcfd/go/src/infra/appengine/test-results/model/aggregate_result.go
[modify] https://crrev.com/662a047497b2f0ab7f7e4466d19efc533305bcfd/go/src/infra/appengine/test-results/model/aggregate_result_test.go

Owner: seanmccullough@chromium.org
Status: Fixed (was: Available)
That was apparently causing a significant fraction of test-results 500s: 
https://screenshot.googleplex.com/G1wC1eKVmDS

Sign in to add a comment