Issue metadata
Sign in to add a comment
|
Gerrit comments in test CL in playground are all at the top of the file |
||||||||||||||||||||||
Issue descriptionExample: https://chromium-review.googlesource.com/c/playground/gerrit-tricium/+/1097599/14/example.txt Expected: Individual trailing space comments should shown be on the related lines. Actual: Everything appears as file-level comment, at the top of the diff.
,
Jun 19 2018
Quick note: Looking at the results stored in Tricium for that request: https://tricium-dev.appspot.com/rpcexplorer/services/tricium.Tricium/Results?request={%22runId%22:%225731880649359360%22} Note that there are no line or columns numbers there. By comparison, this is the results for another recent job, outside the playground repo: https://tricium-dev.appspot.com/rpcexplorer/services/tricium.Tricium/Results?request={%22runId%22:%225649723763458048%22}
,
Jun 29 2018
Note: Spacey reports the right line numbers: 2018/06/29 22:46:42 Wrote RESULTS data, path: "/b/s/w/io1qaMHq/tricium/data/results.json", value: comments:<category:"Spacey/TrailingSpace" message:"Found trailing space" path:"go/src/infra/tricium/functions/spacey/test/README.md" start_line:3 end_line:3 start_char:37 end_char:37 suggestions:<description:"Get rid of trailing space" replacements:<path:"go/src/infra/tricium/functions/spacey/test/README.md" replacement:"Here I add aline with trailing space." start_line:3 end_line:3 end_char:37 > > > comments:<category:"Spacey/TrailingSpace" message:"Found trailing space" path:"go/src/infra/tricium/functions/spacey/test/README.md" start_line:7 end_line:7 start_char:31 end_char:31 suggestions:<description:"Get rid of trailing space" replacements:<path:"go/src/infra/tricium/functions/spacey/test/README.md" replacement:"This line has a trailing space." start_line:7 end_line:7 end_char:31 > > > BUT, by the time the comment gets stored and a request is sent to Gerrit, the line numbers are missing: [gerrit] JSON body: {"notify":"NONE","robot_comments":{"go/src/infra/tricium/functions/spacey/test/README.md":[{"robot_id":"Spacey/TrailingSpace","robot_run_id":"5732063453904896","url":"https://tricium-dev.appspot.com/run/5732063453904896","properties":{"tricium_comment_uuid":"68155e70-954a-41be-9c8a-04118ec9290a"},"id":"","path":"go/src/infra/tricium/functions/spacey/test/README.md","message":"Found trailing space"},{"robot_id":"Spacey/TrailingSpace","robot_run_id":"5732063453904896","url":"https://tricium-dev.appspot.com/run/5732063453904896","properties":{"tricium_comment_uuid":"8c6979f0-ce6b-444f-89c2-6a0116a83272"},"id":"","path":"go/src/infra/tricium/functions/spacey/test/README.md","message":"Found trailing space"}]},"tag":"autogenerated:tricium"} Hypothesis: https://chromium-review.googlesource.com/1099688 changed JSON encoding of input/output from analyzers to use the jsonpb marshaller. But when the results from analyzers are unmarshaled, they're still using the json marshaller, which behaves differently for multi-word keys (i.e. it works for "message", but not for "startLine"). Proposed solution (if the above hypothesis is true): Use jsonpb marshaler for tracker/rpc_worker_done.go
,
Jun 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/11e9aa4fb9e1b6ada266a3e6902279b927f27acb commit 11e9aa4fb9e1b6ada266a3e6902279b927f27acb Author: Quinten Yearsley <qyearsley@chromium.org> Date: Sat Jun 30 21:24:21 2018 [tricium] Use jsonpb Marshaler for Data_Comment In https://chromium-review.googlesource.com/1099688 I changed the JSON encoding of input/output from functions to use the jsonpb marshaler, but I didn't change it everywhere. I believe this has resulted in some fields not being unmarshaled (including startLine, etc.), which has resulted in crbug.com/853887 . Bug: 853887 Change-Id: I91000b89adcc17cf12ab4c071ef85d2835d5a569 Reviewed-on: https://chromium-review.googlesource.com/1121425 Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> [modify] https://crrev.com/11e9aa4fb9e1b6ada266a3e6902279b927f27acb/go/src/infra/tricium/appengine/frontend/rpc_results_test.go [modify] https://crrev.com/11e9aa4fb9e1b6ada266a3e6902279b927f27acb/go/src/infra/tricium/appengine/tracker/rpc_worker_done.go [modify] https://crrev.com/11e9aa4fb9e1b6ada266a3e6902279b927f27acb/go/src/infra/tricium/appengine/tracker/rpc_worker_done_test.go [modify] https://crrev.com/11e9aa4fb9e1b6ada266a3e6902279b927f27acb/go/src/infra/tricium/appengine/gerrit/rpc_report_results_test.go
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/ef4aa6ff7c6af17ad515c821d216085baea9c334 commit ef4aa6ff7c6af17ad515c821d216085baea9c334 Author: Quinten Yearsley <qyearsley@chromium.org> Date: Tue Jul 03 18:27:26 2018 Unmarshal comments with jsonpb when reporting results In https://crrev.com/c/1121425 I changed some JSON serialization from the standard json library to jsonpb, but it was still being decoded using the standard json library. What this means is that line was being encoded as startLine, but when decoding it was looking for start_line, and finding nothing. Bug: 853887 Change-Id: I5054f7bfad74723b0f2749dd34ecd3e05debf4e9 Reviewed-on: https://chromium-review.googlesource.com/1123647 Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> [modify] https://crrev.com/ef4aa6ff7c6af17ad515c821d216085baea9c334/go/src/infra/tricium/appengine/frontend/rpc_results.go [modify] https://crrev.com/ef4aa6ff7c6af17ad515c821d216085baea9c334/go/src/infra/tricium/appengine/gerrit/rpc_report_results.go [modify] https://crrev.com/ef4aa6ff7c6af17ad515c821d216085baea9c334/go/src/infra/tricium/appengine/gerrit/rpc_report_results_test.go
,
Jul 6
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/e1c4e4123028f5527975f68ee64f65b605a3d847 commit e1c4e4123028f5527975f68ee64f65b605a3d847 Author: Quinten Yearsley <qyearsley@chromium.org> Date: Fri Jul 06 20:49:24 2018 [Tricium] Also use jsonpb also when unmarshaling comments in gerrit.go This is the 3rd in a series of CLs changing JSON marshaling: - First: https://crrev.com/c/1121425 - Second: https://crrev.com/c/1123647 After the second CL but before this CL, line numbers are stored in track.Comment but not reported to Gerrit; the reason (again) was because the standard json marshaler was used to unmarshal comments before sending them. This CL changes that. This CL also includes minor variable name changes for the purpose of improving consistency and clarity. Bug: 853887 Change-Id: I3337b03a81316842b1b9c3d6af95fd14d3e87cdc Reviewed-on: https://chromium-review.googlesource.com/1127351 Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> [modify] https://crrev.com/e1c4e4123028f5527975f68ee64f65b605a3d847/go/src/infra/tricium/appengine/gerrit/rpc_report_results.go [modify] https://crrev.com/e1c4e4123028f5527975f68ee64f65b605a3d847/go/src/infra/tricium/appengine/gerrit/gerrit.go
,
Jul 6
Finally fixed! Examples: https://chromium-review.googlesource.com/c/infra/infra/+/1121340/25 https://chromium-review.googlesource.com/c/playground/gerrit-tricium/+/1097599/42 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by estaab@chromium.org
, Jun 19 2018Status: Assigned (was: Untriaged)