Bug 69750 – NRWT should handle duplicated expectations

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

[Csaba Osztrogonác]

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

[Kristóf Kosztyó]

Created attachment 110811 [details]
draft patch

It is working fine in local but do you have any idea in connection with this?

[Peter Gal]

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)

[Kristóf Kosztyó]

Created attachment 111611 [details]
proposed fix

[Adam Barth]

I suspect we don't want to make this change, but I've CCed the experts.

[Csaba Osztrogonác]

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

[Dirk Pranke]

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.

[Kristóf Kosztyó]

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

[Dirk Pranke]

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

[Kristóf Kosztyó]

Created attachment 112675 [details]
proposed fix

[Kristóf Kosztyó]

Comment on attachment 112675 [details]
proposed fix

I forgot to run the test_webkitpy, and I broke that.

[Kristóf Kosztyó]

Created attachment 113296 [details]
proposed fix

[Peter Gal]

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

[Kristóf Kosztyó]

(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

[Peter Gal]

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

[Kristóf Kosztyó]

(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

[Kristóf Kosztyó]

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

[Kristóf Kosztyó]

Created attachment 113305 [details]
proposed fix

[Dirk Pranke]

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?

[Kristóf Kosztyó]

(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

[Dirk Pranke]

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

[Kristóf Kosztyó]

Created attachment 114218 [details]
proposed fix with unit test

I made a unit test

[Csaba Osztrogonác]

ping for review. :)

[Dirk Pranke]

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.

[Kristóf Kosztyó]

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.

[Dirk Pranke]

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.

[Kristóf Kosztyó]

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

[Csaba Osztrogonác]

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.

[Dirk Pranke]

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.

[Kristóf Kosztyó]

Created attachment 117663 [details]
proposed fix

Now it raise a ParseError if it is run in lint mode.

[Dirk Pranke]

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.

[Kristóf Kosztyó]

I changed the name of the list

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