New issue
Advanced search Search tips

Issue 876855 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 23
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Renaming an annotation doesn't fail on CQ

Project Member Reported by nicolaso@chromium.org, Aug 22

Issue description

A recent CL [1] renamed an annotation in annotations.xml, but not in the code (likely a typo). This caused a mismatch between the code and the XML file. It should have failed on CQ, but passed just fine [2].

For some reason, however, the buildbot [3] started failing, even though the CQ check worked. This particular failure should be fixed now [4], but we should try to reproduce and fix the underlying issue.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1153969
[2] https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8938195931929827872/+/steps/check_network_annotations__with_patch_/0/stdout
[3] https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-annotator-rel
[4] https://chromium-review.googlesource.com/c/chromium/src/+/1184110
 
Thank you for noticing and fast response.
To perform the CQ tests fast, traffic_annotation_auditor only processes changed .cc (and .mm) files from the CL that include terms related to annotations. In this specific CL, the related .cc file was not changed and only annotations.xml was changed and hence the mistake went unnoticed and only found in CLs that trigger checking the .cc file that includes this annotation.
(And the CL was not reviewed by an owner of the annotations.xml, considering changes trivial.)

Currently the auditor does not receive the changes and only acts on the list of the changed files. If you can pass the changes to the auditor as well, you can add the following heuristic:
If annotations.xml is changed, and the file name of the changed annotation is not in the change list, auditor can run a full test instead of just a partial test to ensure that the change is valid (which most probably is not).

P.S.,
In the same CL ([1]), 3 annotations are removed, which is also not correct and if the change list would be passable, this will also be checked and prevented.
Owner: syoussefi@chromium.org
I suggest increasing the priority, just caught another CL that was wrongly modifying annotations.xml without changing any files. This will result in corrupted log and test breakage for later CLs.
Labels: -Pri-3 Pri-2
Owner: nicolaso@chromium.org
Status: Assigned (was: Untriaged)
syoussefi@ has been working on this, but was only with our team for 1 week. I'll pick up where he left off.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46548710be35a0f1dc4ddffff8114a58ecb25c64

commit 46548710be35a0f1dc4ddffff8114a58ecb25c64
Author: Nicolas Ouellet-payeur <nicolaso@chromium.org>
Date: Tue Oct 23 13:36:52 2018

Fail on CQ when annotation names don't match between code and XML

- When annotations.xml is modified in a CL, run a full test of traffic
  annotations to catch more errors.

- When there are duplicate hash codes (e.g. after renaming in
  annotations.xml), fail even if error-resilient mode is on.

These 2 things combined means the buildbot will fail on CQ instead of
only failing on the waterfall.

Bug:  876855 
Change-Id: Ib4ada288f4aa29943d1413bd81c4ec7c22735fc6
Reviewed-on: https://chromium-review.googlesource.com/c/1291813
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601926}
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/auditor_result.cc
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/auditor_result.h
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/traffic_annotation_auditor.cc
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/traffic_annotation_auditor.h
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/traffic_annotation_exporter.cc
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/auditor/traffic_annotation_exporter.h
[modify] https://crrev.com/46548710be35a0f1dc4ddffff8114a58ecb25c64/tools/traffic_annotation/scripts/check_annotations.py

Status: Fixed (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67bfc6eb83ca4d877c886493ca03d2ce616412fe

commit 67bfc6eb83ca4d877c886493ca03d2ce616412fe
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Wed Oct 24 07:37:03 2018

Roll traffic_annotation_auditor binaries.

Traffic annotation auditor binaries for Windows and Linux are updated
after the change crrev.com/c/1291813.

Bug:  876855 
Change-Id: I7490b23e192e4c02ca753cae5f3762758deccfb2
TBR: nicolaso@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1297358
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602270}
[modify] https://crrev.com/67bfc6eb83ca4d877c886493ca03d2ce616412fe/tools/traffic_annotation/bin/README.md
[modify] https://crrev.com/67bfc6eb83ca4d877c886493ca03d2ce616412fe/tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1
[modify] https://crrev.com/67bfc6eb83ca4d877c886493ca03d2ce616412fe/tools/traffic_annotation/bin/win32/traffic_annotation_auditor.exe.sha1

Sign in to add a comment