Summary: | NRWT runs some tests that are skipped with -i command line option | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
Component: | Tools / Tests | Assignee: | 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
Alexey Proskuryakov
2012-03-19 12:09:01 PDT
agreed. I was lazy when I implemented this the first time, and was hoping people wouldn't run into it ;). Created attachment 132677 [details]
Patch
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. Created attachment 132682 [details]
reuse add_expectation() instead
(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! Committed r111261: <http://trac.webkit.org/changeset/111261> |