[webkit-reviews] review denied: [Bug 69750] NRWT should handle duplicated expectations : [Attachment 111611] proposed fix

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Kristóf Kosztyó
<kkristof at inf.u-szeged.hu>'s request for review:
Bug 69750: NRWT should handle duplicated expectations
https://bugs.webkit.org/show_bug.cgi?id=69750

Attachment 111611: proposed fix
https://bugs.webkit.org/attachment.cgi?id=111611&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
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.