Bug 97699 – The style bot spams about skia_test_expectations.txt

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

The style bot spams about skia_test_expectations.txt

[Adam Barth]

Created attachment 165830 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 165830 [details]
Patch

We could also downgrade this to an info?  But seems OK to me.  Should let Dirk comment.

[Dirk Pranke]

Comment on attachment 165830 [details]
Patch

I'm not okay with just removing this message altogether, since it can catch real issues with people interactively uploading patches and running the style checks (or doing other things when parsing the chromium files). 

I thought we discussed this somewhere a couple days ago ... I agree that we should fix this so that the style bot doesn't get this warning, but removing the message altogether isn't the right way to do it.

[Adam Barth]

I'm open to suggestions about how to make the style bot not spam.  Our earlier discussion did not result in the problem being fixed.

[Dirk Pranke]

I am sympathetic to wanting to stop the bleeding ... give me a few minutes to suggest an alternative; if there isn't a straightforward way to do that we can land this until I can come up with a better mechanism.

[Adam Barth]

Thanks!  (See https://bugs.webkit.org/show_bug.cgi?id=97013#c24 for a recent example of spamming.)

[Dirk Pranke]

Created attachment 165837 [details]
Patch

[Dirk Pranke]

Okay, I've posted a patch that's closer to what I had in mind, but I'm not totally happy with it. The main thing is that I would like there to be a warning when check-webkit-style is run interactively (or during webkit-patch upload), but not on the EWS bot, and I'm not sure yet how to accomplish this. Also, the patch needs tests and it's generally crufty so I'm not sure if this is worth the effort.

To Eric's comment on the other bug ... what is the style bot's threshold for failures: warnings or errors? I would've hoped it would be errors (since that's my normal definition of the difference between an error and a warning), but if it's not feasible to not have warnings turn the bot red, maybe a better workaround would be to just change this message to _log.info().

If neither of those suggestions will fly, then I'm okay with getting rid of the message altogether, temporarily, but can we do that by commenting out the line and adding a fixme referencing this bug so that I can be reminded to figure out a better way to deal with this? I do agree that the bot shouldn't get this message at all but I'm still working on the best way to make that happen.

[Eric Seidel (no email)]

It's unclear to me what this warning does?

It is an error to run check-webkit-style in WebKit w/o a full chromium checkout?

It's warning you that you haven't run update-webkit --chromium, no?

Why does this have any bearing on style (or the expectations)?

(I don't have enough information here to know if any of this is good or bad.)

[Adam Barth]

It seems unfortunate to need to teach all the other ports about the Chromium-specific notion of a partial checkout.

[Dirk Pranke]

(In reply to comment #9)
> It's unclear to me what this warning does?
> 

It's warning the user that they might not get the results they're expecting because they don't have a properly configured checkout.

> It is an error to run check-webkit-style in WebKit w/o a full chromium checkout?
>

It's not supposed to be an "error" using my definition above, i.e., it shouldn't stop you from running things. It's debatable whether you can properly lint the TestExpectations files if you don't have a full set of them, but I think you should be able to just lint the lines you've changed (which is what check-webkit-style is supposed to be doing, I think), so I'm okay with the bots not needing a full checkout and not producing this warning.

In the interactive user case, I haven't decided if we should have the error or not. In once sense, if a user is modifying the chromium TestExpectations file and doesn't have a full checkout, I'm a bit worried and perhaps the warning is a good thing. In another sense, this should be okay because we should only complain about the lines the user changed.
 
> It's warning you that you haven't run update-webkit --chromium, no?

Right.
 
> Why does this have any bearing on style (or the expectations)?
> 

See above. If you don't have a full chromium checkout, the expectations for the chromium ports may be "wrong": at least, not what you think they might be.


(In reply to comment #10)
> It seems unfortunate to need to teach all the other ports about the Chromium-specific notion of a partial checkout.

Agreed. That's one of the reasons why I'm not totally happy with my patch and okay with landing the other one now that I've taken a look at things. I'm also not happy that I  had to touch so many files; it seems like that indicates that something isn't designed well.

[Adam Barth]

You should be able to change Chromium's TestExpectations file without a DEPS-enabled checkout.  We change the TestExpectations files for other ports all the time, and don't need any of their secret sauce.

[Dirk Pranke]

(In reply to comment #12)
> You should be able to change Chromium's TestExpectations file without a DEPS-enabled checkout.  We change the TestExpectations files for other ports all the time, and don't need any of their secret sauce.

I'm not sure where (or even if) we're disagreeing here. Here are some examples of my thinking, in order of when I think errors are appropriate ...

1.  "run-webkit-tests --chromium" should produce a warning if the skia test expectations file is missing. Arguably, this shouldn't normally happen since one would've had to do a checkout to do a build, but if the file is missing this likely indicates that something's wrong.

2. run-webkit-test --lint-test-files probably should generate a warning, since you may not be seeing a full set of warnings about the chromium ports. I'm not 100% sure about this since I can see arguments for not wanting to get warnings, in some situations, too.

3. As discussed above, check-webkit-style probably should *not* generate a warning, but I'm a bit less certain here because arguably if someone is modifying the chromium files w/o having the downstream files checked out, they might be doing the wrong thing (although I understand your observation and wish we didn't have to split expectations across the multiple repos).

4. The style bot shouldn't generate an error in this case, since it doesn't have a full checkout and we don't want it to have to have one. Arguably it would be nice to generate a non-fatal warning, but I wouldn't want it to generate the 10+ it seems to do now.

Do you agree with the above?

[Adam Barth]

> Do you agree with the above?

Yes.  Perhaps the solution is to check for the file in a different layer.  For example, we could move the check to the same place we check for wdiff and the like.

[Ojan Vafai]

(In reply to comment #13)
> 2. run-webkit-test --lint-test-files probably should generate a warning, since you may not be seeing a full set of warnings about the chromium ports. I'm not 100% sure about this since I can see arguments for not wanting to get warnings, in some situations, too.

I don't think it should warn unless you pass --chromium or something. Having it always spam out this line for people without a Chromium DEPS checkout makes it more likely that they'll learn to ignore the lint output.

[Dirk Pranke]

I suppose one could argue that this warning is doing more harm than good at this point; if the only place we want a warning for sure in case #1, that's also the least likely to actually happen. Thinking about this more over the past couple hours, I think I've convinced myself that it's not that useful a warning at this point (I think it was more useful when things were brand new and the existence of the skia file was not as well known).

I will rework the code a bit and post another try. Something should definitely warn if the file doesn't exist and we're actually trying to run the tests, I think.

[Dirk Pranke]

Created attachment 165901 [details]
Patch

[Dirk Pranke]

Okay, I'm giving up on this warning for now. I've posted a patch that is basically Adam's path with some more comments and some cleaned up test code that will no longer be necessary.

[Adam Barth]

> Okay, I'm giving up on this warning for now. I've posted a patch that is basically Adam's path with some more comments and some cleaned up test code that will no longer be necessary.

Thanks.  :)

[Dirk Pranke]

Committed r129714: <http://trac.webkit.org/changeset/129714>