Only wptserve and its dependencies are needed to run the layout tests. It seems we already tried to avoid importing pytest for example, but the wrong path is used in import-expectations.json. Instead of skipping what we don't want, I think it's better to skip tools and import only what we need, that way if new directories are added upstream we won't import them. This will reduce a lot the size of the tools directory, and will avoid duplication with the WebDriver tests importer that needs wptrunner, webdriver and pytest from tools directory.
Created attachment 321530 [details] Patch Before: LayoutTests/imported/w3c/web-platform-tests $ du -hs tools 12M tools After: LayoutTests/imported/w3c/web-platform-tests $ du -hs tools 2.6M tools
I have no idea why the patch doesn't apply. I tried to apply it in a different machine using git am and it failed too, even when both are at the very same revision. However, git apply and patch both apply the patch correctly. So, I've no idea what's going on. I've run the tests without problems, confirming that's all we need from tools, but it would have been better if EWS had run the tests.
Some tools might be useful though, like WebDriver to run manual tests. The wpt script is not needed but might help as well. py/pytest might indeed be skipped. As of the reason why it is not applying, that is indeed odd. Maybe you can break the patch in smaller pieces to see why it is not applying?
(In reply to youenn fablet from comment #3) > Some tools might be useful though, like WebDriver to run manual tests. WebDriver will be imported in WebriverTests directory. Other tools might be added on demand if needed. > The wpt script is not needed but might help as well. What wpt script? you mean wptrunner? > py/pytest might indeed be skipped. > > As of the reason why it is not applying, that is indeed odd. > Maybe you can break the patch in smaller pieces to see why it is not > applying? Sure, I can try.
(In reply to Carlos Garcia Campos from comment #4) > (In reply to youenn fablet from comment #3) > > Some tools might be useful though, like WebDriver to run manual tests. > > WebDriver will be imported in WebriverTests directory. Other tools might be > added on demand if needed. I haven't really followed how web-platform-tests manual tests will be moved to use web driver. I would expect these tests to stay in web-platform-tests folder. > > The wpt script is not needed but might help as well. > > What wpt script? you mean wptrunner? The wpt script at the root of web-platform-test folder.
(In reply to youenn fablet from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > (In reply to youenn fablet from comment #3) > > > Some tools might be useful though, like WebDriver to run manual tests. > > > > WebDriver will be imported in WebriverTests directory. Other tools might be > > added on demand if needed. > > I haven't really followed how web-platform-tests manual tests will be moved > to use web driver. I would expect these tests to stay in web-platform-tests > folder. I don't mean manual tests, but the WebDriver protocol tests themselves. See bug #177354. > > > The wpt script is not needed but might help as well. > > > > What wpt script? you mean wptrunner? > > The wpt script at the root of web-platform-test folder. That would mean importing also wpt dir and its dependencies.
I managed to apply the patch with git am --keep-cr so the problem seems to be that some files have windows line endings, that at some point are stripped before passing the diff to patch, and it fails because they diffs don't match
Created attachment 321645 [details] Patch for EWS This is the same patch but without the files containing crlf (fortunately they are just a few), for EWS.
Comment on attachment 321645 [details] Patch for EWS There must be something else svn-apply doesn't like :-( because this patch applies fine with git am.
I tried this manually in a mac and tests are working fine. We could probably land this and if we need to add more tools thing in the future we can simply add them on demand.
I am not against removing them. That will bloat a little bit history but this will probably save some history if not reimporting them for some time. Can you verify that reimporting the tests is not readding files you removed. It seems for instance "web-platform-tests/tools/html5lib" would be imported but the patch is deleting "web-platform-tests/tools/html5lib/tests". I would also change the changelog so as to just write which folders are removed, not each individual file.
(In reply to youenn fablet from comment #11) > I am not against removing them. > That will bloat a little bit history but this will probably save some > history if not reimporting them for some time. And disk space! :-) > Can you verify that reimporting the tests is not readding files you removed. > It seems for instance "web-platform-tests/tools/html5lib" would be imported > but the patch is deleting "web-platform-tests/tools/html5lib/tests". I used the import command to check what to remove. html5lib was imported before, but it was not needed, it will be imported by the web driver importer one, though. I'll double check in any case. > I would also change the changelog so as to just write which folders are > removed, not each individual file. Ok, I tried to avoid some noise.
r=me given that bots (EWS ideally but otherwise build.webkit.org bots) are happy.
Committed r222773: <http://trac.webkit.org/changeset/222773>
<rdar://problem/34786228>
(In reply to Carlos Garcia Campos from comment #14) > Committed r222773: <http://trac.webkit.org/changeset/222773> I think this change is responsible for the WPT server error seen here: https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/builds/4010/steps/layout-test/logs/stdio
(In reply to Ryan Haddad from comment #16) > (In reply to Carlos Garcia Campos from comment #14) > > Committed r222773: <http://trac.webkit.org/changeset/222773> > I think this change is responsible for the WPT server error seen here: > https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/ > builds/4010/steps/layout-test/logs/stdio I'm not sure, I would be surprised if it worked in all the bots, but not in that one, either we have the required files to run the server or not.