Hi, I'm importing css-grid-1 test suite from csswg-test repo.
One of the tests that is currently passing has a special output:
https://test.csswg.org/shepherd/testcase/grid-support-grid-template-areas-001/spec/css-grid-1/name/grid-support-grid-template-areas-001/
I guess the issue is that it has two lines like this:
PASS 'grid' with: grid-template-areas: "a b"
"c d";
I guess that might be considered like a failure for testharness.js.
What should we do regarding this?
One option would be to change the test, so it avoids the "new lines";
but I'm wondering if we should find a proper solution for this.
Another temporal option could be to write the "-expected" manually.
Cc: -qyears...@chromium.org Owner: qyears...@chromium.org Status: Started (was: Untriaged)
I think that in this case it makes sense to update our logic that says whether a testharness test is considered "passing" - right now, if there are any "unexpected" lines that aren't pass or fail or console messages, that's considered a fail, but we could change the logic to accept that:
https://codereview.chromium.org/2668973002
Good point -- Technically, I think a testharness.js test with one PASS plus console messages should be considered an "all-pass testharness.js test result", so this change was intended -- although I didn't make a PSA.
I think that in general for testharness.js, console logs should be for diagnostic purposes, and shouldn't determine whether the test passes or fails; this is how results for testharness tests in web-platform-tests are treated in Mozilla. Furthermore, based on the discussion in bug 679742, I think that we actually want to change the behavior of the test runner to totally ignore all parts of testharness.js test output that are not PASS/FAIL results for specific subtests; I think that for testharness.js tests we don't want to look at console messages.
But, after taking a look at what tests are affected, I see that there are hundreds of affected tests (https://codereview.chromium.org/2696083004). Next step: start a discussion on blink-dev, and reach a conclusion about whether console messages in testharness test results can be ignored.
It has just become a little bit more difficult to revert that CL now, since hundreds of new tests have just been imported (in external/wpt) with only a single PASS.
After starting a discussion on blink-dev[1] (a little later than I should have), it seems like it's probably OK to go-ahead with this new rule. Jason, do you think it's OK to not revert this change, and to go forward with the new rule?
If so, the next step is to clean up the all-PASS -expected.txt files.
[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs
The list of baselines that would now be removed according to the new rules about passing testharness tests is in https://codereview.chromium.org/2702133002.
Inspecting these tests individually, I think that:
The tests for these baselines should be updated to use asserts:
csspaint/invalidation-background-image-expected.txt
csspaint/invalidation-border-image-expected.txt
csspaint/invalidation-content-image-expected.txt
The definition of all-PASS should be changed to exclude tests with harness errors so that these are kept:
external/wpt/streams/piping/pipe-through.serviceworker.https-expected.txt
external/wpt/streams/piping/pipe-through.sharedworker-expected.txt
fast/dom/promise-rejection-events-console-expected.txt
fast/dom/promise-rejection-events-lifetime-expected.txt
These should just be removed now:
external/wpt/dom/nodes/Node-removeChild-expected.txt
external/wpt/html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling-expected.txt
external/wpt/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange-expected.txt
And these should maybe be removed, but also maybe the tests should be modified:
fast/css/getComputedStyle/getComputedStyle-backgroundImage-expected.txt
http/tests/feature-policy/vibrate-enabledforself-expected.txt
http/tests/loading/css-no-cache-revalidation-expected.txt
http/tests/loading/doc-write-async-third-party-script-expected.txt
http/tests/loading/download_only_supported_stylesheet_types-expected.txt
http/tests/loading/image-picture-download-after-shrink-expected.txt
http/tests/loading/image-picture-no-download-after-picture-removal-expected.txt
http/tests/loading/image-picture-no-download-after-removal-expected.txt
http/tests/loading/image-picture-no-download-after-source-removal-expected.txt
http/tests/security/vibrate_in_same_origin_iframe_allowed-expected.txt
platform/android/fast/text/hyphens/can-hyphenate-locale-expected.txt
platform/android/fast/text/hyphens/hyphens-parsing-001-expected.txt
platform/mac/fast/text/hyphens/can-hyphenate-locale-expected.txt
Labels: Test-Layout Summary: Change the definition of all PASS for testharness tests to ignore console logging (was: testharness.js test with all PASS is considered to be failing)
Before considering this "done", I think that I should follow up with the things in #c12.
Labels: -Type-Bug Type-Feature Summary: Change the definition of all PASS for testharness tests to ignore console logging. (was: Change the definition of all PASS for testharness tests to ignore console logging)
Comment 1 by r...@igalia.com
, Feb 1 20172.7 KB
2.7 KB View Download