|Summary:||there should be a separate --lint-test-files step on the build bots|
|Product:||WebKit||Reporter:||Dirk Pranke <dpranke>|
|Component:||Tools / Tests||Assignee:||Dirk Pranke <dpranke>|
|Severity:||Normal||CC:||abarth, eric, levin, ojan, rniwa, tony|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Dirk Pranke 2011-12-16 15:45:46 PST
I want to do this for two reasons: We seem to get a fair number of build failures from bad or duplicate expectations on the bots. It may be that our style checker is buggy, but I also suspect a fair number of expectations changes get checked in unreviewed, let alone un-CQ'ed. I'm not sure if webkit-patch land-cowboy tries to run the style checks on this, or if that would catch issues (or if people would just ignore or miss the errors). Second, rniwa has largely convinced me that having duplicate expectations shouldn't be a fatal error on the layout_tests step, but I want to make sure that we don't just start ignoring bad expectations files. Unfortunately, buildbot doesn't give us too many ways to express different kinds of failures, so it seems cleanest to me to break checking the expectations out as a separate step. I think we should be able to tune this so this check is fast and adding a separate step doesn't slow this down by more than a few seconds (I believe linting is slower than this today, so it might need some profiling first). I think if linting the file takes more than (say) 10-15 seconds/run, it probably shouldn't be needed. We can also probably tweak things so that we only ran this if something under LayoutTests/ changed in this build, if we had to. What do others think? Is this worth doing, or should we just try to beef up the presubmit checks if we're missing something?
Comment 1 Ryosuke Niwa 2011-12-16 15:49:42 PST
(In reply to comment #0) > We can also probably tweak things so that we only ran this if something under LayoutTests/ changed in this build, if we had to. > > What do others think? Is this worth doing, or should we just try to beef up the presubmit checks if we're missing something? This is an excellent idea.
Comment 2 Ojan Vafai 2011-12-18 10:32:55 PST
Sounds great. Best of both worlds.
Comment 3 Dirk Pranke 2012-03-12 18:20:08 PDT
This step currently exists on the chromium canaries (and deps bots). Since chromium is the only port actively using test expectations, and since most chromium devs watch the canaries more often than the build.webkit.org bots, and since it's not clear if other ports will start using test_expectations in anything like their current form any time soon ... I'm gonna call this closed.