Bug 152685 – W3C test importer should have an option to clean the destination directory

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 268182 [details]
Patch

[Daniel Bates]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 268272 [details]
Keeping hidden files

[WebKit Commit Bot]

Comment on attachment 268272 [details]
Keeping hidden files

Clearing flags on attachment: 268272

Committed r197163: <http://trac.webkit.org/changeset/197163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.