Bug 188069 – [Curl] Set correct source info to ResourceResponse.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

ResourceResponseBase::Source::Network must be set by each ports.

[Basuke Suzuki]

Created attachment 345899 [details]
PATCH

[Fujii Hironori]

https://trac.webkit.org/changeset/234277/

In the above unreviewd gardening commit, you enabled just only
http/tests/cache/disk-cache/disk-cache-302-status-code.html just
before this patch.

This isn't what I asked you to do. I want you to enable test
cases of http as much as possible. That doesn't mean you should
make all tests pass. It just mean marking failed tests as Failure,
Crash or Timeout.

Can you enable all tests under http/tests/cache/disk-cache?
If not, why?
And, if you can't enable all tests under http/tests/cache/disk-cache.
I don't like the idea enable just only http/tests/cache/disk-cache/disk-cache-302-status-code.html because we can't check regressions by landing your patch.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #2)
> https://trac.webkit.org/changeset/234277/
> 
> In the above unreviewd gardening commit, you enabled just only
> http/tests/cache/disk-cache/disk-cache-302-status-code.html just
> before this patch.
> 
> This isn't what I asked you to do. I want you to enable test
> cases of http as much as possible. That doesn't mean you should
> make all tests pass. It just mean marking failed tests as Failure,
> Crash or Timeout.
> 
> Can you enable all tests under http/tests/cache/disk-cache?
> If not, why?
> And, if you can't enable all tests under http/tests/cache/disk-cache.
> I don't like the idea enable just only
> http/tests/cache/disk-cache/disk-cache-302-status-code.html because we can't
> check regressions by landing your patch.

As I said here https://bugs.webkit.org/show_bug.cgi?id=188033, it is unpractical to maintain the area currently we are not focusing. There are so many errors occur to http test right now. The test set is glowing every day and every time new test added to those area, we have to maintain those.

We are new comer and not ready for entire test yet. We are looking for the bug which makes the number of those errors (failure/crash/timeout) drastically small and fixing them one by one. The one for DumpRenderTree crashing is the one of the case.

Still in such situation, if the fix make the test passing, that should be explicitly noted. That becomes the logging of the regression. I don't think that is meaningless. I don't understand why you say:

> we can't check regressions by landing your patch.

like that.

BTW, we cannot enable disk-cache for DRT because disk-cache itself is disabled. 
https://bugs.webkit.org/show_bug.cgi?id=173361

The test I chose is not related directly with disk-cache, but that uses internal.source interface which is related to this patch.

[Fujii Hironori]

I know it's hard to keep test bots green. That's the reason all
test bots other than Mac port are red all the time. It's not
problem WinCairo test bots will be red. It's important that we
can notice regressions by checking the number of test failures.

There are two reasons I think it's important to enable http tests
for WinCairo port:

1. Curl part is used only by WinCairo port. Other part, for example Cairo part, are tested in other ports.
2. You are actively developing Curl part.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #4)
> I know it's hard to keep test bots green. That's the reason all
> test bots other than Mac port are red all the time. It's not
> problem WinCairo test bots will be red. It's important that we
> can notice regressions by checking the number of test failures.
> 
> There are two reasons I think it's important to enable http tests
> for WinCairo port:
> 
> 1. Curl part is used only by WinCairo port. Other part, for example Cairo
> part, are tested in other ports.
> 2. You are actively developing Curl part.

I understand test is very important. I understand other ports doing such hard maintenance duty every day. We, WinCairo port, want to follow them. That's why we work hard to send these patches.

I'm just saying this is too early to make http tests at this moment because we are still development phase of test environment. We want to maintain the minimum set of tests which can test each feature of TestRunner without trouble.

So how about this? Enabling http/tests. Then disabling sub directories. Does this make sense for you?

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #5)
> So how about this? Enabling http/tests. Then disabling sub directories. Does
> this make sense for you?

Sounds good. I did it <https://trac.webkit.org/r234309>, and enable small portion of http tests. I hope this doesn't cause any trouble for you.

[EWS Watchlist]

Comment on attachment 345899 [details]
PATCH

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

New failing tests:
http/tests/security/canvas-remote-read-remote-video-hls.html

[EWS Watchlist]

Created attachment 345911 [details]
Archive of layout-test-results from ews200 for win-future

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

[WebKit Commit Bot]

Comment on attachment 345899 [details]
PATCH

Rejecting attachment 345899 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 345899, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=345899&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=188069&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 345899 from bug 188069.
Fetching: https://bugs.webkit.org/attachment.cgi?id=345899
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Fujii Hironori']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 4 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/wincairo/TestExpectations
Hunk #1 FAILED at 863.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/wincairo/TestExpectations.rej
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Fujii Hironori']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8672419

[Basuke Suzuki]

Created attachment 345916 [details]
PATCH

Fujii-san, thanks for r+

[WebKit Commit Bot]

Comment on attachment 345916 [details]
PATCH

Clearing flags on attachment: 345916

Committed r234311: <https://trac.webkit.org/changeset/234311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42664177>