Bug 190618 – Make fast/frames/detached-frame-property.html testharness-based

• Component: Tools / Tests
• Status: NEW

[youenn fablet]

Make fast/frames/detached-frame-property.html testharness-based

[youenn fablet]

Created attachment 352455 [details]
Patch

[youenn fablet]

Created attachment 352456 [details]
Patch

[Chris Dumez]

Comment on attachment 352456 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        Instead, rely on one async test and sync test for each assertion.

What's the value of making this change?

This is basically implementing some of the js-test API on top of testharness. The new test no longer looks like a js-test but still does not look like a test harness test either. I do not think we would want to merge a test that looks like this into the WPT repository, would we?

[youenn fablet]

Created attachment 352459 [details]
Two versions

[youenn fablet]

> > LayoutTests/ChangeLog:9
> > +        Instead, rely on one async test and sync test for each assertion.
> 
> What's the value of making this change?

Experimenting on how to author more testharness tests in WebKit.
This is not worth committing this change as is.

> This is basically implementing some of the js-test API on top of
> testharness. The new test no longer looks like a js-test but still does not
> look like a test harness test either. I do not think we would want to merge
> a test that looks like this into the WPT repository, would we?

I don't know, probably not that specific version, but maybe something close.
As long as it is testharness, it can be run on all platforms.

I uploaded an alternate version of the same test that is academic testharness.js.

This is clearly no more difficult (I would even say easier, no more wrapping into strings) than jstest to author but has some drawbacks.
- It stops after the first failing assertion.
- It is a bit more difficult to debug. If an assert_true fails, one needs to check the stack trace to locate the failing assertion.

[Alexey Proskuryakov]

Do you plan to also move the test? This one benefits from svn history particularly, as the rationale for what happened with that fix is not obvious at all.

Other than that, this seems to make the test more complicated, and thus harder to reason about.

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #6)
> Do you plan to also move the test? This one benefits from svn history
> particularly, as the rationale for what happened with that fix is not
> obvious at all.

This is more in preparation for discussing with WPT folks.
I do not plan to move this particular test, upstream it to WPT or even commit this particular change.
I am more interested in seeing new WebKit tests be authored as WPT tests and making that as easy as possible.

> Other than that, this seems to make the test more complicated, and thus
> harder to reason about.

Can you be more precise about what makes you think this is more complicated?

The first version of the test is a two line change.
The second version of the test is basically replacing shouldBeTrue("XXX") by assert_true(XXX).

[Alexey Proskuryakov]

Looking through the patch in detail, here are my comments:

1. Lost TEST COMPLETE in detached-frame-property-expected.txt. Test description somehow went into a PASS line.

2. detached-frame-property.html uses three external scripts instead of one. That's slower and harder to understand.

3. detached-frame-property-testharness-version-expected.txt doesn't have individual step results, which makes it extremely hard to reason about failures. Perhaps I'm just assuming the worst, but in fact it has detailed results when there is a failure? But even so, the behavior would be a regression, as the change in results can't be seen on a diff.

4. Two external script imported by it. One is better than two. It probably doesn't matter for this particular test, but for anything that involves subresource loading, harness subresources are a huge annoyance when debugging.

There's also testharness.css loaded dynamically, but it seems like that happens after the test, so less of a problem for debugging. Should be included into a single subresource for performance anyway.

5. Test description is still on a PASS line, grammatically incorrect ("PASS Checks the value of detached subframe properties").

6. It appears that these assert functions raise an exception on failure, meaning that only one failure per test can be reported. Is that correct?

[youenn fablet]

Thanks for the feedback.

(In reply to Alexey Proskuryakov from comment #8)
> Looking through the patch in detail, here are my comments:
> 
> 1. Lost TEST COMPLETE in detached-frame-property-expected.txt. Test
> description somehow went into a PASS line.

I guess this is mostly an habit and am not sure whether that is worth addressing.
If proved to be useful, it will be very easy to add such line through testharnessreport.js.

> 2. detached-frame-property.html uses three external scripts instead of one.
> That's slower and harder to understand.

I don't see how harder to understand this is.
I agree that we could categorize that as boilerplate, which is a minor concern for authoring.
It would be easy to just include one script if that is helping authoring.
.window.js would also help authoring.

As of slower, we can do some measurements.
I agree it will be slower since it will parse JS code that will never be used at all.
Data being cached, I am not sure the difference will be huge though.

From your feedback so far, I do not see how the modified detached-frame-property.html is more difficult to understand.

> 3. detached-frame-property-testharness-version-expected.txt doesn't have
> individual step results, which makes it extremely hard to reason about
> failures. Perhaps I'm just assuming the worst, but in fact it has detailed
> results when there is a failure? But even so, the behavior would be a
> regression, as the change in results can't be seen on a diff.

This is a valid concern that is identified, that this patch tries to demonstrate and hopefully can be fixed in testharness.js.
In case of failure, it has detailed error results so one always knows which assertion actually failed.
This is less easy to read than with js-test as the assertion is identified by its line in JS code.

> 4. Two external script imported by it. One is better than two. It probably
> doesn't matter for this particular test, but for anything that involves
> subresource loading, harness subresources are a huge annoyance when
> debugging.

I don't really understand that.
If one is better than two, that would call for writing inline test without any testharness at all.
Once you need to wait pass the load of one JS file, you are already annoyed when doing step-by-step debugging.
If doing stderr-style debugging, I don't think this matters much.

> There's also testharness.css loaded dynamically, but it seems like that
> happens after the test, so less of a problem for debugging. Should be
> included into a single subresource for performance anyway.
> 
> 5. Test description is still on a PASS line, grammatically incorrect ("PASS
> Checks the value of detached subframe properties").

This can be corrected pretty easily.

> 6. It appears that these assert functions raise an exception on failure,
> meaning that only one failure per test can be reported. Is that correct?

Agreed, and this is a concern that would be nice to get addressed in testharness.js

[Alexey Proskuryakov]

I'm glad that you agree with some of these.

TEST COMPLETED is there to make it easily visible that the test did run to completion (e.g. no uncaught exception). Not important when looking at a regression in a carefully written test, but mass imported ones can have problems like this slip in (and that happened in the past).

As for why clicking through more unneeded breakpoint stops is worse than clicking through fewer, or why analyzing three external scripts is harder than analyzing one, I don't know what to say. I certainly understand that myself.

[youenn fablet]

Created attachment 353372 [details]
Patch

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #10)
> I'm glad that you agree with some of these.
> 
> TEST COMPLETED is there to make it easily visible that the test did run to
> completion (e.g. no uncaught exception). Not important when looking at a
> regression in a carefully written test, but mass imported ones can have
> problems like this slip in (and that happened in the past).

testharnessreport.js provides similar information and I believe it could be mocked quite easily.

> As for why clicking through more unneeded breakpoint stops is worse than
> clicking through fewer, or why analyzing three external scripts is harder
> than analyzing one, I don't know what to say. I certainly understand that
> myself.

Sure, but the thing is that whatever happens, we have to handle it anyway since most tests at least load one main resource and one subresource before starting anything meaningful.
We could find solutions here (JS inlining, JS bundling).
I am not convinced this is worth the effort though.

From my past experience with WPT service worker, which are doing loads, my main issue is that many tests run in one file, which adds a lot more noise (tens of subresource loads for instance).

[EWS Watchlist]

Comment on attachment 353372 [details]
Patch

Attachment 353372 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9780194

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353374 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353372 [details]
Patch

Attachment 353372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9780211

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353376 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353372 [details]
Patch

Attachment 353372 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9780196

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353378 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353372 [details]
Patch

Attachment 353372 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9780360

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353381 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 353372 [details]
Patch

Attachment 353372 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9780312

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 353384 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6