Bug 69750 - NRWT should handle duplicated expectations
Summary: NRWT should handle duplicated expectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kristóf Kosztyó
URL:
Keywords:
Depends on:
Blocks: 64491
  Show dependency treegraph
 
Reported: 2011-10-10 03:21 PDT by Csaba Osztrogonác
Modified: 2011-12-05 01:49 PST (History)
8 users (show)

See Also:


Attachments
draft patch (1.69 KB, patch)
2011-10-13 01:45 PDT, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
proposed fix (2.16 KB, patch)
2011-10-19 07:06 PDT, Kristóf Kosztyó
dpranke: review-
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (3.89 KB, patch)
2011-10-27 06:44 PDT, Kristóf Kosztyó
kkristof: review-
kkristof: commit-queue-
Details | Formatted Diff | Diff
proposed fix (6.38 KB, text/plain)
2011-11-02 02:51 PDT, Kristóf Kosztyó
no flags Details
proposed fix (6.42 KB, patch)
2011-11-02 05:45 PDT, Kristóf Kosztyó
dpranke: review-
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix with unit test (8.66 KB, patch)
2011-11-09 00:34 PST, Kristóf Kosztyó
dpranke: review-
Details | Formatted Diff | Diff
proposed fix (8.56 KB, patch)
2011-11-26 10:06 PST, Kristóf Kosztyó
dpranke: review-
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (9.41 KB, patch)
2011-12-02 11:45 PST, Kristóf Kosztyó
dpranke: review+
dpranke: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-10-10 03:21:48 PDT
Now NRWT exits before testing if a test is in the Skipped file
and somebody add an entry for it into test_expectations.py:

http://build.webkit.org/builders/Qt%20Linux%20Release/builds/38286:
2011-10-07 10:47:22,765 24230 test_expectations.py:914 ERROR Line:167 Duplicate or ambiguous expectation. fast/forms/input-disabled-color.html

Skipped file entry must be stronger than an entry in test_expectations.txt
Comment 1 Kristóf Kosztyó 2011-10-13 01:45:49 PDT
Created attachment 110811 [details]
draft patch

It is working fine in local but do you have any idea in connection with this?
Comment 2 Peter Gal 2011-10-13 04:20:21 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=110811&action=review

Some notes regarding the code:

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:347
> +        index = 0;
> +        for line in expectations.split('\n'):

you should use enumerate(expectations.split('\n')) that will give an additional index value also (http://docs.python.org/library/functions.html#enumerate), so you can ditch the previous index initialization and the index +=1 below. (just a side note: you don't need the semicolon :))

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354
> +            if match and match.group('TEST') not in tests.keys():
> +                tests[match.group('TEST')] = index
> +            elif match and 'SKIP' in match.group('MODIFIER'):
> +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %
> +                         (match.group('TEST'),tests[match.group('TEST')] + 1))
> +                tests[match.group('TEST')] = index

something feels weird here but I don't know yet what..

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:359
> +        tests_list = tests.values().sort()
> +        for line in sorted(tests.values()):
> +            result += (expectations.split('\n')[line]) + '\n'
> +        return result

Is it necessary to sourt the test.values() twice?
Also you preform a split on the expectations on each iteration, you could move the split out of the for loop.

A better solution would be:
result_list = [expectations_list[line] for line in sorted(test.values())]
return '\n'.join(result_list)
Comment 3 Kristóf Kosztyó 2011-10-19 07:06:55 PDT
Created attachment 111611 [details]
proposed fix
Comment 4 Adam Barth 2011-10-19 13:26:57 PDT
I suspect we don't want to make this change, but I've CCed the experts.
Comment 5 Csaba Osztrogonác 2011-10-19 13:30:28 PDT
(In reply to comment #4)
> I suspect we don't want to make this change, but I've CCed the experts.

We do want, because Qt layout testing is regularly broken when somebody add 
a test to test_expectations.txt when a test is in the Skipped list already.

Skipped list entry must be stronger than test_expectations.txt entry.
Comment 6 Dirk Pranke 2011-10-19 14:37:30 PDT
Comment on attachment 111611 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350
> +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %

If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* the contents of the test_expectations.txt file, which means that (a) your line numbers are wrong, (b) you will fail if the test is mentioned multiple times in the Skipped files, (c) you will fail if the test is mentioned multiple times in the test_expectations.txt file.

This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least.

In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation and may make it harder to evolve things in the future. The whole concept of handling Skipped files this way in the first place was supposed to be a temporary hack until we could transition everyone to test_expectations.txt files. Now, I realize that we probably don't have consensus over whether we even want to do that, or how, but we should probably fix that instead of continuing to work around it here. At the very least, I would be inclined to change the code so that the port object can return a list of tests to skip, and the generic code can decide how to merge the skip lists with any test_expectations.

All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped file and the test_expectations.txt file, because such a situation indicates that you don't have a consistent view of what you expect the tests to do.

So perhaps I'm not following what situation you are trying to handle? 

Sorry! I'm sure if I can get a better understanding of what you're trying to fix / workaround we can figure out a way to make it work that I'll be happy with.
Comment 7 Kristóf Kosztyó 2011-10-20 10:58:44 PDT
(In reply to comment #6)
> (From update of attachment 111611 [details]) View in context:
> https://bugs.webkit.org/attachment.cgi?id=111611&action=review
>
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350
> > +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %
>
> If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* th$
(a) the line number is good because the list from the Skipped files is appended at the end of the expectations string
    expectations = self._filesystem.read_text_file(expectations_path) + expectations
(b) the list from the Skipped files is came as a set
    skipped_layout_tests()
(c) the warn is written in multiple times in that case. I thought it is better than if I write it once.
>
> This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least.
>
> In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation a$
I agree with your opinion, thank you for the r-, it needs a sophisticated solution.
I think the merging should be in the test_expectations.py, what do you think?
>
> All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped $
>
> So perhaps I'm not following what situation you are trying to handle?
>
we can't use test_expectations at now because some qt port still have to be run with orwt so we have a great list in the Skipped files, if a careless gardener use that it can broke the nrwt
other example the test_expectations doesn't support the different qt versions so we need to use their Skipped if some test start to fail with it (qt-4.8, qt-5.0)
> Sorry! I'm sure if I can get a better understanding of what you're trying to fix / workaround we can figure out a way to make it work that I'll be$
Comment 8 Dirk Pranke 2011-10-20 19:42:27 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 111611 [details] [details]) View in context:
> > https://bugs.webkit.org/attachment.cgi?id=111611&action=review
> >
> > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:350
> > > +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %
> >
> > If I'm following this patch correctly, the expectations that are passed in contain the concatenation of the contents of the Skipped files *and* th$
> (a) the line number is good because the list from the Skipped files is appended at the end of the expectations string
>     expectations = self._filesystem.read_text_file(expectations_path) + expectations

Ah, right.

> (b) the list from the Skipped files is came as a set
>     skipped_layout_tests()

Okay.

> (c) the warn is written in multiple times in that case. I thought it is better than if I write it once.

That's probably okay.

> >
> > This patch should have tests for all of these cases - I'm r-'ing this patch for this reason at least.
> >
> > In addition, I *really* don't like embedding knowledge of the syntax of the expectations file into the port/* classes; it's a layering violation a$
> I agree with your opinion, thank you for the r-, it needs a sophisticated solution.
> I think the merging should be in the test_expectations.py, what do you think?

Yes, that would be the right place for it.

> >
> > All that said, I don't think I even agree with the basic premise. I believe that it should be an error if the test is mentioned in both a Skipped $
> >
> > So perhaps I'm not following what situation you are trying to handle?
> >
> we can't use test_expectations at now because some qt port still have to be run with orwt so we have a great list in the Skipped files, if a careless gardener use that it can broke the nrwt

The gardener should run new-run-webkit-tests --lint-test-files in order to ensure that it doesn't break; this is no different from handling any other typo or duplicate. If there is a conflict in the test_expectations.txt file, the gardener should be able to comment out or remove the line at that point.

> other example the test_expectations doesn't support the different qt versions so we need to use their Skipped if some test start to fail with it (qt-4.8, qt-5.0)

In this case, we should just add support for the qt versions to your port's implementation in NRWT. That should be very easy to do.
Comment 9 Kristóf Kosztyó 2011-10-27 06:44:12 PDT
Created attachment 112675 [details]
proposed fix
Comment 10 Kristóf Kosztyó 2011-10-27 06:49:49 PDT
Comment on attachment 112675 [details]
proposed fix

I forgot to run the test_webkitpy, and I broke that.
Comment 11 Kristóf Kosztyó 2011-11-02 02:51:58 PDT
Created attachment 113296 [details]
proposed fix
Comment 12 Peter Gal 2011-11-02 03:27:36 PDT
(In reply to comment #11)
> Created an attachment (id=113296) [details]
> proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362
> +    def skipped_test(self):
> +        return skipped_layout_tests

Where does this 'skipped_layout_tests' comes from?
Comment 13 Kristóf Kosztyó 2011-11-02 03:47:31 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=113296) [details] [details]
> > proposed fix
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=113296&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362
> > +    def skipped_test(self):
> > +        return skipped_layout_tests
> 
> Where does this 'skipped_layout_tests' comes from?

On WebKitPort based ports the skipped_layout_tests() return with a set of the skipped tests, but in the Chromium this method use a test_expectation object, but I want to use this set in the test_expectation's constructor to add skipped lines
so I have to make a new method in the webkit.py what only return with the set of skipped tests to make the unittest happy, and don't kill the nrwt when it run chromium
Comment 14 Peter Gal 2011-11-02 04:10:22 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created an attachment (id=113296) [details] [details] [details]
> > > proposed fix
> > 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113296&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:362
> > > +    def skipped_test(self):
> > > +        return skipped_layout_tests
> > 
> > Where does this 'skipped_layout_tests' comes from?
> 
> On WebKitPort based ports the skipped_layout_tests() return with a set of the skipped tests, but in the Chromium this method use a test_expectation object, but I want to use this set in the test_expectation's constructor to add skipped lines
> so I have to make a new method in the webkit.py what only return with the set of skipped tests to make the unittest happy, and don't kill the nrwt when it run chromium

But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something?
Comment 15 Kristóf Kosztyó 2011-11-02 05:00:18 PDT
(In reply to comment #14)
> But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something?

Indeed I was wrong, but strange it didn't throw any exception
Comment 16 Kristóf Kosztyó 2011-11-02 05:29:25 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > But this is not a method call, also if that's the case the it should be 'self.skipped_layout_tests()', or am I missing something?
> 
> Indeed I was wrong, but strange it didn't throw any exception

Now I got it why didn't throw anything, I'am fixing it :)
Comment 17 Kristóf Kosztyó 2011-11-02 05:45:08 PDT
Created attachment 113305 [details]
proposed fix
Comment 18 Dirk Pranke 2011-11-03 18:29:16 PDT
Comment on attachment 113305 [details]
proposed fix

This is definitely getting closer ...

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703
> +        self._add_skipped_tests(port.skipped_tests())

Can't we just use port.skipped_layout_tests() here and not have to add a separate function?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:836
> +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %

As per earlier discussion, I think this should probably be an error, not a warning.

Also, can you add some unit tests to the test_expectations_unittest.py class?
Comment 19 Kristóf Kosztyó 2011-11-04 06:23:43 PDT
(In reply to comment #18)
> (From update of attachment 113305 [details])
> This is definitely getting closer ...
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=113305&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703
> > +        self._add_skipped_tests(port.skipped_tests())
> 
> Can't we just use port.skipped_layout_tests() here and not have to add a separate function?

In the chromium port the skipped_layout_tests() use a TestExpectation object so it fall in infinite recursive loop

> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:836
> > +                _log.warn('The %s test from test_expectations.txt in line %d is also in Skipped!' %
> 
> As per earlier discussion, I think this should probably be an error, not a warning.
> 

Ok, then it will be an error.

> Also, can you add some unit tests to the test_expectations_unittest.py class?

Sure
Comment 20 Dirk Pranke 2011-11-04 17:34:52 PDT
(In reply to comment #19)
> > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:703
> > > +        self._add_skipped_tests(port.skipped_tests())
> > 
> > Can't we just use port.skipped_layout_tests() here and not have to add a separate function?
> 
> In the chromium port the skipped_layout_tests() use a TestExpectation object so it fall in infinite recursive loop
> 

Oh yeah, that call returns the list of skipped tests including the tests skipped in test_expectations.txt; that call shouldn't be on the port object at all. Oh well, that's a separate bug. Your solution is fine for now.
Comment 21 Kristóf Kosztyó 2011-11-09 00:34:08 PST
Created attachment 114218 [details]
proposed fix with unit test

I made a unit test
Comment 22 Csaba Osztrogonác 2011-11-21 02:58:29 PST
ping for review. :)
Comment 23 Dirk Pranke 2011-11-21 12:44:17 PST
Comment on attachment 114218 [details]
proposed fix with unit test

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

Sorry, I'm not sure how this fell off of my plate.

Almost there, I would just simplify the unit test a bit.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:269
> +        self.assertEqual(logs_string, 'The fast/foo/bar.html test from test_expectations.txt in line 2 is also in Skipped!\n')

I don't usually test for exact string matches on the error messages; that makes the tests fragile. I would just test that parsing the expectations raises a ParseError, as in test_overrides__duplicate on line 226.
Comment 24 Kristóf Kosztyó 2011-11-26 10:06:54 PST
Created attachment 116659 [details]
proposed fix

Hi
I simplified the unit test a little,
but it doesn't throw any exception just log it as error.
Comment 25 Dirk Pranke 2011-11-28 15:13:18 PST
Comment on attachment 116659 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:837
> +                            (test.name, index))

This needs to add an error to a list of errors, not call _log.error(); that way it can be included in the total list of errors during the call to _report_errors. That's why a ParseError() isn't getting raised. 

Unfortunately, the refactoring of this class has made adding an error harder than it used to be, as there isn't a global list-of-errors object. maybe create a self.skipped_tests_errors, and check that in self._report_errors()?

Also, if these are truly errors (and not warnings), you should probably not be adding them to the model.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:263
> +        self._exp = TestExpectations(port, 'failures/expected/text.html\n', 'BUGX : failures/expected/text.html = text\n', None)

This should be raising a ParseError; see comments above.
Comment 26 Kristóf Kosztyó 2011-11-29 08:07:46 PST
(In reply to comment #25)
> (From update of attachment 116659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116659&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:837
> > +                            (test.name, index))
> 
> This needs to add an error to a list of errors, not call _log.error(); that way it can be included in the total list of errors during the call to _report_errors. That's why a ParseError() isn't getting raised. 
> 
> Unfortunately, the refactoring of this class has made adding an error harder than it used to be, as there isn't a global list-of-errors object. maybe create a self.skipped_tests_errors, and check that in self._report_errors()?
> 

Ok, I see

> Also, if these are truly errors (and not warnings), you should probably not be adding them to the model.
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:263
> > +        self._exp = TestExpectations(port, 'failures/expected/text.html\n', 'BUGX : failures/expected/text.html = text\n', None)
> 
> This should be raising a ParseError; see comments above.
Comment 27 Csaba Osztrogonác 2011-12-01 23:47:09 PST
One more accident why we really need this change as soon as possible:
http://trac.webkit.org/changeset/101745

Kristóf, could you speed up fixing this bug, please? Maybe it would be more effective if you guys continue reviewing/fixing this patch with IRC talking instead of waiting days for each other. Thanks your work in advance.
Comment 28 Dirk Pranke 2011-12-02 11:01:19 PST
I can see if I can post a patch in the next day or two that addresses my last concerns, if Kristóf doesn't get to it first.
Comment 29 Kristóf Kosztyó 2011-12-02 11:45:00 PST
Created attachment 117663 [details]
proposed fix

Now it raise a ParseError if it is run in lint mode.
Comment 30 Dirk Pranke 2011-12-02 14:58:22 PST
Comment on attachment 117663 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:791
> +

Minor nit ... _is_lint_mode makes warnings errors. If you want these to fail only in lint mode, I'd just change these to _skipped_tests_warnings and append them to the warnings list. CQ-'ing just for that.

There is actually a separate discussion about making the expectations errors less fatal going on in https://bugs.webkit.org/show_bug.cgi?id=73603 ; fixing that bug might've made this bug unnecessary, but I think there's good work in this patch to clean up our handling of skipped files regardless.
Comment 31 Kristóf Kosztyó 2011-12-05 01:39:13 PST
I changed the name of the list

patch landed in: http://trac.webkit.org/changeset/101979