Bug 81535

Summary: NRWT runs some tests that are skipped with -i command line option
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, jberlin, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
reuse add_expectation() instead none

Description Alexey Proskuryakov 2012-03-19 12:09:01 PDT
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.
Comment 1 Dirk Pranke 2012-03-19 12:11:22 PDT
agreed. I was lazy when I implemented this the first time, and was hoping people wouldn't run into it ;).
Comment 2 Dirk Pranke 2012-03-19 14:46:32 PDT
Created attachment 132677 [details]
Patch
Comment 3 Ojan Vafai 2012-03-19 14:51:46 PDT
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.
Comment 4 Dirk Pranke 2012-03-19 15:16:21 PDT
Created attachment 132682 [details]
reuse add_expectation() instead
Comment 5 Dirk Pranke 2012-03-19 15:17:24 PDT
(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!
Comment 6 Dirk Pranke 2012-03-19 15:43:09 PDT
Committed r111261: <http://trac.webkit.org/changeset/111261>