Bug 152685 - W3C test importer should have an option to clean the destination directory
Summary: W3C test importer should have an option to clean the destination directory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-04 03:02 PST by youenn fablet
Modified: 2016-02-26 02:10 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.34 KB, patch)
2016-01-04 05:56 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Keeping hidden files (9.86 KB, patch)
2016-01-05 00:46 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-01-04 03:02:32 PST
When regularly importing tests from W3C to WebKit, the process is usually:
1. Remove all files, except -expected.txt files from LayoutTests/imported/w3c/web-platform-tests
2. Import test using test importer
3. Remove -expected.txt files that no longer have a corresponding file.

It would be nice for test importer to automate step 1 and 3.
Comment 1 youenn fablet 2016-01-04 05:56:30 PST
Created attachment 268182 [details]
Patch
Comment 2 Daniel Bates 2016-01-04 10:29:38 PST
Comment on attachment 268182 [details]
Patch

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

r-'ing this patch because it will delete files in .svn directories and .gitattributes files.

> Tools/Scripts/webkitpy/w3c/test_importer.py:235
> +    def _should_not_keep_when_importing(self, filesystem, dirname, filename):

Would it make sense to keep all hidden files? At the very least we should also keep .gitattributes files and .svn directories. 

I also noticed in my checkout of /LayoutTests/imported/w3c/web-platform-tests I have files .tools.url and .resources.url. I take it these are generated and OK to delete.

> Tools/Scripts/webkitpy/w3c/test_importer.py:237
> +            return False

It tends to be more difficult to reason about negations and concepts like "not keeping a file" than "keeping a file". I suggest either changing the meaning of this function such that it returns whether to "keep a file when importing" or adding inline comments on this line (line 237), line 239, and line 240 to better describe the implications of the return value. Such comments could be the form "Keep", "Keep", "Do not keep", respectively.

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:199
> +            '/mock-checkout/WebKitBuild/w3c-tests/csswg-tests/test.html': '1',

Can we add tests for .gitignore, .gitattributes, and .svn directories?
Comment 3 youenn fablet 2016-01-05 00:34:28 PST
Thanks for the review.

> r-'ing this patch because it will delete files in .svn directories and
> .gitattributes files.

Right, I will fix that.

> > Tools/Scripts/webkitpy/w3c/test_importer.py:235
> > +    def _should_not_keep_when_importing(self, filesystem, dirname, filename):
> 
> Would it make sense to keep all hidden files? At the very least we should
> also keep .gitattributes files and .svn directories. 
> 
> I also noticed in my checkout of
> /LayoutTests/imported/w3c/web-platform-tests I have files .tools.url and
> .resources.url. I take it these are generated and OK to delete.

It is ok to delete them right.
But it does not hurt keeping them either.
I will change the routine to keep all hidden files.

> > Tools/Scripts/webkitpy/w3c/test_importer.py:237
> > +            return False
> 
> It tends to be more difficult to reason about negations and concepts like
> "not keeping a file" than "keeping a file". I suggest either changing the
> meaning of this function such that it returns whether to "keep a file when
> importing" or adding inline comments on this line (line 237), line 239, and
> line 240 to better describe the implications of the return value. Such
> comments could be the form "Keep", "Keep", "Do not keep", respectively.

Right.
I will change the routine name to _should_remove_before_importing.
Then it is clearer.

> 
> > Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:199
> > +            '/mock-checkout/WebKitBuild/w3c-tests/csswg-tests/test.html': '1',
> 
> Can we add tests for .gitignore, .gitattributes, and .svn directories?

Will add it.
Comment 4 youenn fablet 2016-01-05 00:46:21 PST
Created attachment 268272 [details]
Keeping hidden files
Comment 5 WebKit Commit Bot 2016-02-26 02:10:02 PST
Comment on attachment 268272 [details]
Keeping hidden files

Clearing flags on attachment: 268272

Committed r197163: <http://trac.webkit.org/changeset/197163>
Comment 6 WebKit Commit Bot 2016-02-26 02:10:06 PST
All reviewed patches have been landed.  Closing bug.