Bug 74762
| 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> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | abarth, eric, levin, ojan, rniwa, tony |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 73603 | ||
Dirk Pranke
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?
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Ryosuke Niwa
(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.
Ojan Vafai
Sounds great. Best of both worlds.
Dirk Pranke
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.