New issue
Advanced search Search tips

Issue 869103 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

_CheckCrbugLinksHaveHttps applied too broadly

Project Member Reported by wychen@chromium.org, Jul 30

Issue description

The PRESUBMIT function _CheckCrbugLinksHaveHttps wrongly check the crbug.com URLs against all changed files. It should only check source files.
 
The current white list is _IMPLEMENTATION_EXTENSIONS. I think it should also include say, Java files. Other scripts might also need to be considered.

The current black list is:
  black_list = (_EXCLUDED_PATHS + _TEST_CODE_EXCLUDED_PATHS)
I think the URL pattern also applies to the testing code as well.

mcasas@, what do you think?
Summary: _CheckCrbugLinksHaveHttps applied too broadly (was: _CheckCrbugLinksHaveHttps)
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 31

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

commit b1ce35493ba38e9f3934b6640ac151b0bd31a0e5
Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Date: Tue Jul 31 02:37:01 2018

Check parameter types in FilterSourceFile() mock

Parameters |white_list| and |black_list| in FilterSourceFile()
should be iterables, and it's fairly easy to wrongly pass a string
instead.

This checking caught _CheckCrbugLinksHaveHttps in unit tests.

Bug:  869103 
Change-Id: I1cd6d62c3fa2b1500d9d7b6b35794f40e35378af
Reviewed-on: https://chromium-review.googlesource.com/1155379
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579288}
[modify] https://crrev.com/b1ce35493ba38e9f3934b6640ac151b0bd31a0e5/PRESUBMIT.py
[modify] https://crrev.com/b1ce35493ba38e9f3934b6640ac151b0bd31a0e5/PRESUBMIT_test_mocks.py

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 8

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

commit ce74dfc98f20583fd5d5c98f7635cf2cefecb642
Author: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Date: Wed Aug 08 01:15:18 2018

Fix scope of _CheckCrbugLinksHaveHttps

The current white list is _IMPLEMENTATION_EXTENSIONS. The white list
should also include Java files and other scripts.

The current black list is:
  black_list = (_EXCLUDED_PATHS + _TEST_CODE_EXCLUDED_PATHS)
The URL pattern should also apply to the testing code.

Bug:  869103 
Change-Id: Idbf67580fda198359ce687ecb0dfef5c0546b088
Reviewed-on: https://chromium-review.googlesource.com/1155712
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581424}
[modify] https://crrev.com/ce74dfc98f20583fd5d5c98f7635cf2cefecb642/PRESUBMIT.py
[modify] https://crrev.com/ce74dfc98f20583fd5d5c98f7635cf2cefecb642/PRESUBMIT_test.py

Status: Fixed (was: Started)

Sign in to add a comment