Bug 81535 – NRWT runs some tests that are skipped with -i command line option

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

[Alexey Proskuryakov]

I'm seeing some failures in http/tests even when running "run-webkit-tests -i http".

Dirk discovered that this happens when an individual test is listed in text_expectations.txt. Clearly, a command line switch should have precedence over that.

[Dirk Pranke]

agreed. I was lazy when I implemented this the first time, and was hoping people wouldn't run into it ;).

[Dirk Pranke]

Created attachment 132677 [details]
Patch

[Ojan Vafai]

Comment on attachment 132677 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:872
> +            for test in expectation_line.matching_tests:
> +                self._model._clear_expectations_for_test(test, expectation_line)
> +                self._model._test_to_expectation_line[test] = expectation_line
> +                self._model._add_test(test, expectation_line, overrides_allowed=True)

Could you instead add a boolean to add_expectation_line? Something like overwrite_existing? I'm not a huge fan of duplicating accounting code like this. If the code changes in add_expectation_line, we'd almost certainly want it to change here to.

[Dirk Pranke]

Created attachment 132682 [details]
reuse add_expectation() instead

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 132677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132677&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:872
> > +            for test in expectation_line.matching_tests:
> > +                self._model._clear_expectations_for_test(test, expectation_line)
> > +                self._model._test_to_expectation_line[test] = expectation_line
> > +                self._model._add_test(test, expectation_line, overrides_allowed=True)
> 
> Could you instead add a boolean to add_expectation_line? Something like overwrite_existing? I'm not a huge fan of duplicating accounting code like this. If the code changes in add_expectation_line, we'd almost certainly want it to change here to.

Yeah, I had tried that initially and hadn't been happy with how it looked. I've reworked it again with some different variable names and think this iteration is okay.

Thanks for the review!

[Dirk Pranke]

Committed r111261: <http://trac.webkit.org/changeset/111261>