Bug 73603

Summary: nrwt shouldn't blow up when there are errors in test_expectations.txt
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, levin, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74762    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
rename assert_bad_exp
none
rebase to head. none

Description Ryosuke Niwa 2011-12-01 16:46:18 PST
It's really disrupting for nrwt not to run tests, and being able to cause that by just having one typo in test_expectations.txt is super annoying especially for webkit gardeners.

We should make sure nrwt gracefully fails when encountering such errors (e.g. ignoring lines with errors).
Comment 1 Dirk Pranke 2011-12-01 16:54:03 PST
NRWT fails by design; having errors like duplicate expectations is bad and shouldn't be ignored; in other words, if there is an error in the expectations file, it should fail the build step. But I don't think you're suggesting the errors should be ignored completely (i.e., what would happen if we just turned them into warnings).

I think one problem now is that when this does fail, the reason for the failure isn't obvious on the waterfall ... you get a ??? rather than "bad expectations" or something. It may partially because (IIRC) duplicate expectations actually cause a stack trace rather than an orderly exit.

I think one could argue that a bad expectation, if it was obvious enough, should be sufficient, because it would be quickly corrected.

However, I could also be convinced that NRWT should run what it can unambiguously run and come up with some sort of meaningful failure interpretation.

Another possibility would be to add a new --lint-test-files builds step that turned red separately from the actual test run. Then the test run would be free to treat the parsing failures as warnings and proceed on.

CC'ing Tony and Ojan on this since I believe they've held opinions on this in the past.
Comment 2 Tony Chang 2011-12-01 16:56:25 PST
We should just make it harder to check in failures.  webkit-patch land and webkit-patch land-cowboy should both check to see if test_expectations.txt has changed, if it has, run --lint-test-files.
Comment 3 Ryosuke Niwa 2011-12-01 17:00:30 PST
(In reply to comment #1)
> NRWT fails by design; having errors like duplicate expectations is bad and shouldn't be ignored; in other words, if there is an error in the expectations file, it should fail the build step. But I don't think you're suggesting the errors should be ignored completely (i.e., what would happen if we just turned them into warnings).

No, I'm not suggesting to ignore the failure. I'm just suggesting to make nrwt not stop running tests when encountering errors.

> I think one problem now is that when this does fail, the reason for the failure isn't obvious on the waterfall ... you get a ??? rather than "bad expectations" or something. It may partially because (IIRC) duplicate expectations actually cause a stack trace rather than an orderly exit.

We can just ignore the line and assume it's PASS unless otherwise specified elsewhere.

> Another possibility would be to add a new --lint-test-files builds step that turned red separately from the actual test run. Then the test run would be free to treat the parsing failures as warnings and proceed on.

nrwt can still fail by returning -1 at the end. It's not great to lose the entire test coverage just for one typo in text_expectations.txt.

(In reply to comment #2)
> We should just make it harder to check in failures.  webkit-patch land and webkit-patch land-cowboy should both check to see if test_expectations.txt has changed, if it has, run --lint-test-files.

I don't think that's a good idea (I'll be opposed to such an idea). I don't want webkit-patch land get slowed down by such checks.
Comment 4 Ryosuke Niwa 2011-12-01 17:03:08 PST
> (In reply to comment #2)
> > We should just make it harder to check in failures.  webkit-patch land and webkit-patch land-cowboy should both check to see if test_expectations.txt has changed, if it has, run --lint-test-files.
> 
> I don't think that's a good idea (I'll be opposed to such an idea). I don't want webkit-patch land get slowed down by such checks.

Let me rephrase it. I'll be opposed to this idea because I don't want us to keep adding such checks to webkit-patch land and slow it down. Even today, I use "svn commit" sometimes to avoid having to wait for webkit-patch land to run.
Comment 5 Dirk Pranke 2011-12-01 17:29:51 PST
(In reply to comment #2)
> We should just make it harder to check in failures.  webkit-patch land and webkit-patch land-cowboy should both check to see if test_expectations.txt has changed, if it has, run --lint-test-files.

I strongly support this; I view it as equivalent to a style check. It should no more be acceptable to check in expectation errors than any other kind of quickly-catchable error.

If --lint-test-files is too slow, we can look into speeding it up.
Comment 6 Ryosuke Niwa 2011-12-01 17:35:59 PST
(In reply to comment #5)
> (In reply to comment #2)
> > We should just make it harder to check in failures.  webkit-patch land and webkit-patch land-cowboy should both check to see if test_expectations.txt has changed, if it has, run --lint-test-files.
> 
> I strongly support this; I view it as equivalent to a style check. It should no more be acceptable to check in expectation errors than any other kind of quickly-catchable error.

Sure, but adding things to webkit-patch doesn't help at all if people land patches with svn commit.

Anyhow, I don't think it's a big deal with a bad entry or two as long as bots are turned red because then gardeners can fix those errors in the next iteration.
Comment 7 Ryosuke Niwa 2011-12-01 17:36:57 PST
By the way, as discussed elsewhere, we should get rid of :, =, ordering, etc... grammar we have in test_expectations.txt. It does more harm than it helps.
Comment 8 Eric Seidel (no email) 2011-12-01 22:38:12 PST
I agree.  I don't think it should be a fatal error.
Comment 9 Dirk Pranke 2012-01-19 16:31:34 PST
Created attachment 123221 [details]
Patch
Comment 10 Ryosuke Niwa 2012-01-19 16:37:30 PST
Comment on attachment 123221 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123221&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:134
> +    def assert_bad_exp(self, expectations, overrides=None):

What does "exp" stand here? Please spell out the word.
Comment 11 Dirk Pranke 2012-01-19 16:40:18 PST
Ojan and/or Dimitri, can you please take a look at this as well?

Ojan, Adam, this shows the duplication in testing I was talking about between layout_tests/models/test_expectations_unittests.py and style/checkers/test_expectations_unittest.py.

I think we should just have the style checker test test that we do parse the expectations once and get an error, but there's no need to test all of the different kinds of errors (since the unit tests for the parser themselves catch this). I can either make that change as part of this patch or (preferably) in a follow-up patch.
Comment 12 Ryosuke Niwa 2012-01-19 16:41:49 PST
(In reply to comment #11)
> I think we should just have the style checker test test that we do parse the expectations once and get an error, but there's no need to test all of the different kinds of errors (since the unit tests for the parser themselves catch this). I can either make that change as part of this patch or (preferably) in a follow-up patch.

That sounds like a nice follow up patch.
Comment 13 Dirk Pranke 2012-01-19 16:42:55 PST
Created attachment 123222 [details]
rename assert_bad_exp
Comment 14 Ryosuke Niwa 2012-01-19 16:47:22 PST
I'll let Adam or Ojan review this time as you've requested :)
Comment 15 Dirk Pranke 2012-01-19 16:55:35 PST
(In reply to comment #14)
> I'll let Adam or Ojan review this time as you've requested :)

Thanks for your review as well! The only change between the two revisions is renaming the method as you suggested.
Comment 16 Ryosuke Niwa 2012-01-20 12:05:53 PST
Comment on attachment 123222 [details]
rename assert_bad_exp

Okay, I think Adam and Ojan must have had a chance to look at this. If not, we can always revert it.
Comment 17 Ojan Vafai 2012-01-20 12:29:43 PST
I'm not a fan of doing this before we've added a lint step on the bots. Without that, we'll just get a ton of warnings that people will ignore.
Comment 18 Ojan Vafai 2012-01-20 12:31:26 PST
Also, now that webkit-patch catches these errors, I think it will be extremely rare for people to check them in if they break the tests. If they just give a warning, then people will check them in and learn to ignore the pre-submit warnings.
Comment 19 Dirk Pranke 2012-01-20 13:45:53 PST
(In reply to comment #17)
> I'm not a fan of doing this before we've added a lint step on the bots. Without that, we'll just get a ton of warnings that people will ignore.

I can certainly delay landing this until after we have a lint step on the bots.

(In reply to comment #18)
> Also, now that webkit-patch catches these errors, I think it will be extremely rare for people to check them in if they break the tests. If they just give a warning, then people will check them in and learn to ignore the pre-submit warnings.

I don't think I understand what you're suggesting here. Are you saying these should always be errors? Or that we should keep the current behavior instead of this? Or perhaps these should be errors unless we provide a switch to make them non-fatal (which we would probably want to use on the bots)?
Comment 20 Ojan Vafai 2012-01-20 13:54:55 PST
(In reply to comment #19)
> (In reply to comment #17)
> > I'm not a fan of doing this before we've added a lint step on the bots. Without that, we'll just get a ton of warnings that people will ignore.
> 
> I can certainly delay landing this until after we have a lint step on the bots.

That would be my preference.

> (In reply to comment #18)
> > Also, now that webkit-patch catches these errors, I think it will be extremely rare for people to check them in if they break the tests. If they just give a warning, then people will check them in and learn to ignore the pre-submit warnings.
> 
> I don't think I understand what you're suggesting here. Are you saying these should always be errors? Or that we should keep the current behavior instead of this? Or perhaps these should be errors unless we provide a switch to make them non-fatal (which we would probably want to use on the bots)?

Sorry, I was just being confusing. I'm happy with this patch as is once we add a lint step on the bots.
Comment 21 Dirk Pranke 2012-02-02 13:37:21 PST
Created attachment 125178 [details]
rebase to head.
Comment 22 Dirk Pranke 2012-02-02 13:55:24 PST
Committed r106591: <http://trac.webkit.org/changeset/106591>
Comment 23 Eric Seidel (no email) 2012-02-10 10:53:51 PST
Comment on attachment 125178 [details]
rebase to head.

Cleared review? from attachment 125178 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 24 Dirk Pranke 2012-02-10 16:13:10 PST
that patch landed as per comment #22; not sure why the r? wasn't cleared, but thanks!