Bug 73603 – nrwt shouldn't blow up when there are errors in test_expectations.txt

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

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).

[Dirk Pranke]

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.

[Tony Chang]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

> (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.

[Dirk Pranke]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Eric Seidel (no email)]

I agree.  I don't think it should be a fatal error.

[Dirk Pranke]

Created attachment 123221 [details]
Patch

[Ryosuke Niwa]

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.

[Dirk Pranke]

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.

[Ryosuke Niwa]

(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.

[Dirk Pranke]

Created attachment 123222 [details]
rename assert_bad_exp

[Ryosuke Niwa]

I'll let Adam or Ojan review this time as you've requested :)

[Dirk Pranke]

(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.

[Ryosuke Niwa]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Dirk Pranke]

(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)?

[Ojan Vafai]

(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.

[Dirk Pranke]

Created attachment 125178 [details]
rebase to head.

[Dirk Pranke]

Committed r106591: <http://trac.webkit.org/changeset/106591>

[Eric Seidel (no email)]

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).

[Dirk Pranke]

that patch landed as per comment #22; not sure why the r? wasn't cleared, but thanks!