Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668973002:
Consider any testharness result with >= 1 PASS and no FAIL to be passing. (Closed)
DescriptionConsider any testharness result with >= 1 PASS and no FAIL to be passing.
This changes the rule about testharness test results to make
it less conservative and simpler - so that newlines and text
in the output are OK, and the test is considered passing as
long as there's at least one PASS and no FAIL.
BUG=687492
Review-Url: https://codereview.chromium.org/2668973002
Cr-Commit-Position: refs/heads/master@{#448045}
Committed: https://chromium.googlesource.com/chromium/src/+/a8bd98b223f058a993bbfebbbfd5225544907fda
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update rule for passing testharness test, and update tests #
Total comments: 2
Patch Set 3 : Update test again #
Total comments: 1
Patch Set 4 : Remove two lines from TestExpectations #
Messages
Total messages: 26 (14 generated)
qyearsley@chromium.org changed reviewers: + rego@igalia.com
I'm probably not the best person to review this, so take my suggestions just as possible ideas. https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:71: # considered to be a pass. Shouldn't we require we at least have one "PASS". Dunno really just an idea. https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:74: ' \n' Why an empty line is not allowed but some random text is? I'm not sure if there's a good reason for that.
qyearsley@chromium.org changed reviewers: + jeffcarp@chromium.org
These suggestions make sense - I'll update this CL later. Also added Jeff as a reviewer. https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:71: # considered to be a pass. On 2017/02/01 at 16:16:04, Manuel Rego wrote: > Shouldn't we require we at least have one "PASS". > Dunno really just an idea. That makes sense - assuming that every testharness test should have at least one test function inside of it. https://codereview.chromium.org/2668973002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:74: ' \n' On 2017/02/01 at 16:16:04, Manuel Rego wrote: > Why an empty line is not allowed but some random text is? > I'm not sure if there's a good reason for that. Good point
Description was changed from ========== Consider any testharness result with no fail or console message to be passing. BUG=687492 ========== to ========== Consider any testharness result with >= 1 PASS and no FAIL to be passing. This changes the rule about testharness test results to make it less conservative and simpler - so that newlines and text in the output are OK, and the test is considered passing as long as there's at least one PASS and no FAIL. BUG=687492 ==========
Description was changed from ========== Consider any testharness result with >= 1 PASS and no FAIL to be passing. This changes the rule about testharness test results to make it less conservative and simpler - so that newlines and text in the output are OK, and the test is considered passing as long as there's at least one PASS and no FAIL. BUG=687492 ========== to ========== Consider any testharness result with >= 1 PASS and no FAIL to be passing. This changes the rule about testharness test results to make it less conservative and simpler - so that newlines and text in the output are OK, and the test is considered passing as long as there's at least one PASS and no FAIL. BUG=687492 ==========
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Updated now - does this seem OK?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The change LGTM, but again I'm not an expert on these scripts. BTW, you can remove the entries on TestExpectations related to this bug (the 2 grid tests that were failing); or I'd do it in a follow-up patch. https://codereview.chromium.org/2668973002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): https://codereview.chromium.org/2668973002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:73: # considered to be a pass. I believe the comment here needs to be updated.
> BTW, you can remove the entries on TestExpectations related to this bug (the 2 grid tests that were failing); or I'd do it in a follow-up patch. That should probably be done in this CL. There may also be other tests that have all passes, with newlines, which have -expected.txt files which should now be deleted, OR tests with no PASS or FAIL lines, which previously would be considered passing and now need to have a -expected.txt added. jeffcarp, mlamouri, could you take a look at this CL when you have time later? https://codereview.chromium.org/2668973002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): https://codereview.chromium.org/2668973002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:73: # considered to be a pass. On 2017/02/01 at 19:54:20, Manuel Rego wrote: > I believe the comment here needs to be updated. Updated
I'm lgtm if the trybots pass. https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:72: # If there are no PASS lines, then the test is not considered to pass. I'm wary of this because maybe some tests expect to pass but don't assert anything. I have no data though.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/03 at 03:37:00, jeffcarp wrote: > I'm lgtm if the trybots pass. > > https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): > > https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:72: # If there are no PASS lines, then the test is not considered to pass. > I'm wary of this because maybe some tests expect to pass but don't assert anything. I have no data though. Yeah, actually there's probably a "right" answer here, as far as whether testharness.js tests with no test functions at all are considered "passing". I guess they're passing 0/0 of the test functions, and mathematically speaking 0/0 is "undefined"? I'll update when I know the answer to that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/03 at 04:13:26, qyearsley wrote: > On 2017/02/03 at 03:37:00, jeffcarp wrote: > > I'm lgtm if the trybots pass. > > > > https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py (right): > > > > https://codereview.chromium.org/2668973002/diff/40001/third_party/WebKit/Tool... > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results_unittest.py:72: # If there are no PASS lines, then the test is not considered to pass. > > I'm wary of this because maybe some tests expect to pass but don't assert anything. I have no data though. > > Yeah, actually there's probably a "right" answer here, as far as whether testharness.js tests with no test functions at all are considered "passing". I guess they're passing 0/0 of the test functions, and mathematically speaking 0/0 is "undefined"? I'll update when I know the answer to that. Just tried it, and a test with no test file with no tests appears to have no output, so for now I *think* it should be considered failing.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com, jeffcarp@chromium.org Link to the patchset: https://codereview.chromium.org/2668973002/#ps60001 (title: "Remove two lines from TestExpectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486148891909700,
"parent_rev": "505d7a79e8ee2d03d1abcfa14c0bfe64caa6b627", "commit_rev":
"a8bd98b223f058a993bbfebbbfd5225544907fda"}
Message was sent while issue was closed.
Description was changed from ========== Consider any testharness result with >= 1 PASS and no FAIL to be passing. This changes the rule about testharness test results to make it less conservative and simpler - so that newlines and text in the output are OK, and the test is considered passing as long as there's at least one PASS and no FAIL. BUG=687492 ========== to ========== Consider any testharness result with >= 1 PASS and no FAIL to be passing. This changes the rule about testharness test results to make it less conservative and simpler - so that newlines and text in the output are OK, and the test is considered passing as long as there's at least one PASS and no FAIL. BUG=687492 Review-Url: https://codereview.chromium.org/2668973002 Cr-Commit-Position: refs/heads/master@{#448045} Committed: https://chromium.googlesource.com/chromium/src/+/a8bd98b223f058a993bbfebbbfd5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a8bd98b223f058a993bbfebbbfd5...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2704463003/ by qyearsley@chromium.org. The reason for reverting is: This CL had the effect of causing hundreds of existing baselines to fail presubmit -- see http://crbug.com/687492 (I think I'd still like to reland this later, but after discussion. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
