Bug 50205 - [chromium] new-run-webkit-tests: add better messages, return code for --lint-test-files
Summary: [chromium] new-run-webkit-tests: add better messages, return code for --lint-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-29 20:19 PST by Ojan Vafai
Modified: 2010-12-09 15:22 PST (History)
5 users (show)

See Also:


Attachments
Patch (20.79 KB, patch)
2010-11-30 18:55 PST, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-11-29 20:19:03 PST
Sync to r72870 and run "new-run-webkit-tests --lint-test-files". It should give errors that there are 3 duplicate lines on mac. It doesn't give any errors, but if you actually run the tests, it does. This means the presubmit for test_expectations isn't working.
Comment 1 Dirk Pranke 2010-11-30 16:38:47 PST
When you say "it doesn't give any errors", what are you referring to? for me, it does print messages to stderr, but the return code is 0. check-webkit-style reports the error as well. Is that what you see/saw?
Comment 2 Dirk Pranke 2010-11-30 18:55:54 PST
Created attachment 75241 [details]
Patch
Comment 3 Dirk Pranke 2010-11-30 19:00:50 PST
Per conversation w/ Ojan, it sounds like it was user error with new-run-webkit-tests and either user error with webkit-patch land-cowboy or a bug. For the former, he didn't specify --platform chromium-mac, so the command linted the platform/mac (webkit) version of the file, which was fine. 

Also, he ran land-cowboy, which skips the check-webkit-style presubmit check, which should have reported the error correctly (it does for me when I try to reproduce what he saw).

That said, I'll use this bug as an excuse to clean up the --lint-test-files code and logging. The log output should be cleaner, we no longer use the 'print' statement, and the exception raising is a bit more straightforward. Also, I removed the 'suppress_errors' keyword param to the constructor, which appears to be unused (I don't remember why it was there in the first place).

Also, in reviewing the patch you'll notice that platform/chromium was parsing the expectations files with is_lint_mode=True, and I've changed it to False. True was almost certainly the wrong value, and the new error propagation was causing things to fail in a way that the old code wouldn't (in the face of non-fatal errors).
Comment 4 David Levin 2010-12-03 07:01:37 PST
Sounds like the bug title no longer reflects what this bug is about. It would be best to fix it.
Comment 5 Dirk Pranke 2010-12-03 11:41:01 PST
good idea. done.
Comment 6 Dirk Pranke 2010-12-08 20:29:29 PST
hmm ... this seems to have slipped through the cracks. Can someone please review this?
Comment 7 Dirk Pranke 2010-12-09 15:22:25 PST
Committed r73654: <http://trac.webkit.org/changeset/73654>