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

Issue 764556 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Split lines in SmokeTests; allow and ignore comment lines.

Project Member Reported by qyears...@chromium.org, Sep 12 2017

Issue description

Currently, the SmokeTests file contents are read as one big string, and a test is considered to be in SmokeTests if the test name appears in the file as a substring...

This might potentially lead to some unexpected consequences, e.g. if a/b/c/foo/bar.html is in the file, then b/c/foo/bar.html and foo/bar.html are also considered to be in the file, even if they don't have their own line.

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py?l=934

I would expect the behavior to look at separate lines, and allow comments. Something like:

    tests = {line.strip() for line in contents.splitlines()
             if not line.lstrip().startswith('#')}
    # test is in SmokeTests if it's in this tests set.
 
Labels: -Pri-3 M-63 OS-Android Pri-2
Owner: wolenetz@chromium.org
Status: Started (was: Available)
This is important enough to the media team to prevent coverage regressions (like that fixed in  bug 763528 ) that I'll take a stab at fixing this now (following suggestions in CR thread up to https://chromium-review.googlesource.com/c/chromium/src/+/658356#message-3a50b2c21f284008523d41e1e01c1bc09948c3ad).
Cc: chcunningham@chromium.org dpranke@chromium.org
CL: https://chromium-review.googlesource.com/c/chromium/src/+/665350
Cc: crouleau@chromium.org yini...@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 13 2017

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

commit 6315c77889427fffb84dea412dddc39e6e69b017
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Sep 13 23:10:12 2017

SmokeTests: Allow comments and require Tools OWNERS to review changes

To help prevent accidental SmokeTests coverage regressions like that
fixed in  bug 763528 , this change allows '#' prefixed comments in
SmokeTests, and also makes changes to SmokeTests require Tools OWNERS
review.

BUG= 764556 , 763528 

Change-Id: I612fb2b67fbe9f0a99107c1079b79a4b61338944
Reviewed-on: https://chromium-review.googlesource.com/665350
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501783}
[modify] https://crrev.com/6315c77889427fffb84dea412dddc39e6e69b017/third_party/WebKit/LayoutTests/OWNERS
[modify] https://crrev.com/6315c77889427fffb84dea412dddc39e6e69b017/third_party/WebKit/LayoutTests/SmokeTests
[modify] https://crrev.com/6315c77889427fffb84dea412dddc39e6e69b017/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py

Status: Fixed (was: Started)
#4 should fix this.

Sign in to add a comment