New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 853887 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Gerrit comments in test CL in playground are all at the top of the file

Project Member Reported by qyearsley@google.com, Jun 18 2018

Issue description

Example:
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.
 

Comment 1 by estaab@chromium.org, Jun 19 2018

Owner: qyears...@chromium.org
Status: Assigned (was: Untriaged)
Assigning this, please reassign if needed.
Cc: diegomtzg@google.com
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}
Cc: juliehockett@google.com
Status: Started (was: Assigned)
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
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Sign in to add a comment