Hi, ftp eplf does not handle directory. When trying to parse this example: http://cr.yp.to/ftp/list/eplf.html +i8388621.48594,m825718503,r,s280, djb.html +i8388621.50690,m824255907,/, 514 +i8388621.48598,m824253270,r,s612, 514.html directory 514 is not displayed.
Created attachment 66381 [details] php testcase
that's because of: if (!result.type) /* access denied */ { ... return FTPJunkEntry; } (in file FTPDirectoryParser.cpp, function parseOneFTPLine) actually, FTPDirectoryEntry is 0. So, FTPDirectoryEntry is considered as junk. I don't known why this test is performed only in SUPPORT_EPLF. Is it safe to remove it ?
Created attachment 66385 [details] patch v0.1 first patch proposal: removes access denied test
Created attachment 66386 [details] patch v0.2 second patch proposal: start FTPEntryType enum to 1
Comment on attachment 66385 [details] patch v0.1 webkit-patch upload will correctly obsolete old patches and set r? on new patches for you. Also + No new tests. (OOPS!) Should be replaced with a list of tests for this change, or an explanation of why testing is impossible.
Comment on attachment 66386 [details] patch v0.2 You need to explain "why" this is correct in your ChangeLog. As-is, this untested change makes no sense.
Created attachment 239713 [details] Patch
Comment on attachment 239713 [details] Patch Looks good, r=me.
Comment on attachment 239713 [details] Patch Wait, there seems to be a test case attached to this bug report, are we sure this cannot be layout tested?
Right, I didn't notice it was there and I wasn't aware php could be used in tests. I will look into turning it into a proper layout test but I probably need to get the php-based tests running for the Haiku port first.
Comment on attachment 239713 [details] Patch Thanks for your contribution. We are clearing the review queue of old patches that no longer apply to WebKit trunk. Please resubmit this bug with the test case turned into a layout test. If you need help, ping myself or cdumez on IRC.
Hello, I would like to work on this. I have only found a higher level overview of how testcases are to be written for webkit, is there some specific documentation for cases like this?
Hi, The tests are found in the LayoutTests/ directory. Most of them are html files and are just loaded directly by the browser by accessing the files. In this specific case we need a test that is loaded through a web server. When this patch was first written, this would be done using php, but now it's using python instead. You can find examples of tests like that in LayoutTests/http/tests/. The php file attached to this ticket can be trivially converted to a python script following these examples. Next to each test there is a file with the same name, with the extension replaced with "-expected.txt". This corresponds to the output of the DumpRenderTree executable when loading and executing that specific test. The testsuite frameworks run DumpRenderTree and compares the result with this file, to decide if the test is passed or failed. The expected result (pass or fail) for each test is stored in "TestExpectations" files in the LayoutTests directory. The default is PASS so you don't need to do anything special if the test passes everywhere. There are several expectation files for different platforms and configurations, allowing to keep track of the current state of tests. This make it easy to see the impact of any given changes, both in terms of tests that are broken, and tests that are fixed by it.
Created attachment 447520 [details] Patch
Created attachment 447523 [details] Patch
It looks like I've uploaded the wrong test expectation (from a previous iteration I've tested) which the Macos builder hit, sorry about that. It does not really explain however why the gtk builder has hit an internal server error when trying to run this test. https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/r447520-3351/http/tests/misc/ftp-eplf-directory-pretty-diff.html I don't really know what the GTK port does different to the mac port here, I would apreciate some guidance.
The link above is from the previous attempt, this time it just times out on the GTK builder (while the mac builder(s) seem to run the test normally)
Created attachment 447543 [details] Patch
Hello, A bit of context for this: we don't use it for FTP in HaikuWebKit (yet) but we use it to provide directory listings for the file:// protocol accessing local directories. The EPLF format is used because it is well-specified and easy to generate in our file:// protocol handler. Pascal now converted the attached test to Python and added it to the LayoutTests, but the expectations.txt would need to be different on different platforms. As this isn't an html test, we have no control over the exact rendering of the directory listing (different fonts are used on each platform, changing the size of elements) and we also can't use javascript to do a more specific test. We don't know how to proceed with this, any opinions?
Looking at the MacOS test results it seems there is a second problem, that of the timestamp display. The Haiku port has a patch to change the display of timestamps in the ftp code to use the OS timezone for display instead of gmttime. How does the TestSuite deal with such situations? Is there a predictable timezone set for tests or would for example the python test have to change the send listing based on the current timezone? And as a second question should the code to change the timestamp display be submitted in this same changeset? The diff, for reference: --- a/Source/WebCore/loader/FTPDirectoryParser.cpp +++ b/Source/WebCore/loader/FTPDirectoryParser.cpp @@ -156,7 +156,7 @@ FTPEntryType parseOneFTPLine(const char* line, ListState& state, ListResult& res time_t t = static_cast<time_t>(seconds); // FIXME: This code has the year 2038 bug - gmtime_r(&t, &result.modifiedTime); + localtime_r(&t, &result.modifiedTime); result.modifiedTime.tm_year += 1900; } }
(In reply to Pascal Abresch from comment #16) > It looks like I've uploaded the wrong test expectation (from a previous > iteration I've tested) which the Macos builder hit, sorry about that. > > It does not really explain however why the gtk builder has hit an internal > server error when trying to run this test. > https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/r447520-3351/ > http/tests/misc/ftp-eplf-directory-pretty-diff.html > > I don't really know what the GTK port does different to the mac port here, I > would apreciate some guidance. GTK doesn't enable FTPDIR at build time. So the test is timing out there. I suggest skipping this test at LayoutTests/platform/gtk/TestExpectations
Created attachment 452688 [details] Patch
I've updated the patch to skip the GTK test and include the MacOS test as the expected result.
Created attachment 452729 [details] Patch
The layout test now passes for MacOS, it still fails for iOS duo to the differinh font sizes. How should I proceed with this?
(In reply to Pascal Abresch from comment #25) > The layout test now passes for MacOS, it still fails for iOS duo to the > differinh font sizes. How should I proceed with this? You should add a file in this patch with the iOS test results [1] in the path LayoutTests/platform/ios/http/tests/misc/ftp-eplf-directory-expected.txt [1] https://ews-build.s3-us-west-2.amazonaws.com/iOS-15-Simulator-WK2-Tests-EWS/452729-8760-rerun/http/tests/misc/ftp-eplf-directory-actual.txt
Created attachment 453305 [details] Patch
Comment on attachment 453305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453305&action=review Thanks so much for this contribution. Looks good. > Source/WebCore/ChangeLog:17 > + Test: http/tests/misc/ftp-eplf-directory-expected.txt Ideally this should list the test, ftp-eplf-directory.py, not the expected result file. Really unfortunate there is no way to make an FTP test use text-style results instead of render tree dumps. We’ll have to rebase these expected results any time there are rendering changes (unrelated to FTP).
Created attachment 453307 [details] Patch
I've updated the patch, I agree that it is unfortunate to use the precise rendering output here since the test only wants to roughly test whether the directory is display, and not if it is displayed in the exact same way. Thanks for all the help!
Long ago this was Mozilla source code; I wonder if Firefox has a similar bug. I couldn’t find it myself by searching. Maybe they changed this long ago.
Comment on attachment 453307 [details] Patch Waiting for EWS to show passing results, then someone should set commit-queue+ too.
Looks like ews passed now, it had a (probably unrelated) failure in wk2 ios before, but it worked after a restart. It should be ready to commit now.
Committed r290595 (247871@main): <https://commits.webkit.org/247871@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453307 [details].
<rdar://problem/89555741>