W3C importer should use filesystem instead of shutil/host

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

Tools/Scripts/w3c/test_importer.py should use Host abstraction in lieu of shutil, os...

[youenn fablet]

Created attachment 247322 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 247322 [details]
Patch

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

Does this enable the creation of any new unit tests, or do we have to wait until os.walk is replaced with something mockable?

> Tools/Scripts/webkitpy/w3c/test_importer.py:94
> +    if not FileSystem().exists(import_dir):

Nit: put FileSystem() in a local variable and use that here.

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 247322 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247322&action=review
> 
> Does this enable the creation of any new unit tests, or do we have to wait
> until os.walk is replaced with something mockable?

Good point.
We could already test quite some functionality by creating dedicated TestImporter.import_list and running TestImporter.import_tests.
But these tests may need to be updated whenever we change import_list/import_tests.
I would tend to wait until replacing os.walk to create black-box tests.

> > Tools/Scripts/webkitpy/w3c/test_importer.py:94
> > +    if not FileSystem().exists(import_dir):
> 
> Nit: put FileSystem() in a local variable and use that here.

OK

[youenn fablet]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 247322 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=247322&action=review
> > 
> > Does this enable the creation of any new unit tests, or do we have to wait
> > until os.walk is replaced with something mockable?
> 
> Good point.
> We could already test quite some functionality by creating dedicated
> TestImporter.import_list and running TestImporter.import_tests.
> But these tests may need to be updated whenever we change
> import_list/import_tests.
> I would tend to wait until replacing os.walk to create black-box tests.

With patches from bug 142084 and bug 142085, test importer becomes testable with the mock system. Patch in bug 142085 adds one such unit test to ensure that.

[Bem Jones-Bey]

Comment on attachment 247322 [details]
Patch

Good stuff. r=me.

[youenn fablet]

Created attachment 247612 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 247612 [details]
Patch for landing

Clearing flags on attachment: 247612

Committed r180844: <http://trac.webkit.org/changeset/180844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.