Bug 118156 – Update w3c import script to be able to import to a custom location

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Bem Jones-Bey]

Update location of w3c shapes tests to reflect new import location

[Bem Jones-Bey]

Created attachment 205647 [details]
Patch

Update import location for new import script

[Dirk Pranke]

Comment on attachment 205647 [details]
Patch

There's two problems here.

1) I'd prefer to reserve LayoutTests/w3c for when we're importing the full repos regularly using the script, rather than using piecemeal directories done by individual devs.

2) We need to preserve the upstream dir structure where possible, so it probably needs to be w3c/csswg/submitted/shapes .

[Bem Jones-Bey]

(In reply to comment #2)
> (From update of attachment 205647 [details])
> There's two problems here.
> 
> 1) I'd prefer to reserve LayoutTests/w3c for when we're importing the full repos regularly using the script, rather than using piecemeal directories done by individual devs.

But I would like to have my tests updated by the full import when it happens. Is there a way to do that, or do you think that we should end up with 2 copies of the tests in the tree? Or is there another option that I'm missing?

If I should be importing them to a different location, would you be OK with me adding the ability for the script to do that? I don't really want to have to manually move them every time I import.

> 
> 2) We need to preserve the upstream dir structure where possible, so it probably needs to be w3c/csswg/submitted/shapes .

The script used to import into csswg/submitted/shapes, and it started importing into w3c/shapes after I pulled in all of your changes from Blink. Is this a bug in the script, or am I doing it wrong?

[Dirk Pranke]

Sorry for the delay in the replies, somehow I missed this message.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 205647 [details] [details])
> > There's two problems here.
> > 
> > 1) I'd prefer to reserve LayoutTests/w3c for when we're importing the full repos regularly using the script, rather than using piecemeal directories done by individual devs.
> 
> But I would like to have my tests updated by the full import when it happens. Is there a way to do that, or do you think that we should end up with 2 copies of the tests in the tree? Or is there another option that I'm missing?
> 

At the moment, I don't have a plan for someone to do a piecemeal import now and then have it updated for by a full import later. In theory, if we check the tests directly into the WebKit svn repo, this would be possible, but it would probably also be kinda confusing. In Blink, which will pull the tests in via a sub repo, this won't even be possible.

I would imagine that once we have the full import working and running regularly, we would go back and delete the duplicate copies.

> If I should be importing them to a different location, would you be OK with me adding the ability for the script to do that? I don't really want to have to manually move them every time I import.
>

I'd be totally fine with that. 

> > 
> > 2) We need to preserve the upstream dir structure where possible, so it probably needs to be w3c/csswg/submitted/shapes .
> 
> The script used to import into csswg/submitted/shapes, and it started importing into w3c/shapes after I pulled in all of your changes from Blink. Is this a bug in the script, or am I doing it wrong?

Possibly some of each :). I think that if you only give the script one argument, it assume that that is a top-level directory (e.g., approved/, submitted/). If you give it two arguments, then you should be able specify the subtree, e.g., import-w3c-tests csswg/submitted/shares csswg/ .

This could probably be improved to make the script more aware of how to find the top of the repo.

[Bem Jones-Bey]

Created attachment 206418 [details]
Patch

First attempt at enabling partial imports to a special directory.

[Dirk Pranke]

Comment on attachment 206418 [details]
Patch

looks promising, but I need to stare at it some more when I have more time. I'll try to find that later tonight if I can.

[Dirk Pranke]

Comment on attachment 206418 [details]
Patch

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

Sorry for the delay in reviewing :(.

I have a few comments as I don't understand why you're making some of the changes you are making. The code basically looks okay otherwise.

> Tools/ChangeLog:18
> +            no longer true, and I didn't see a good way to keep that behavior.

I'm not sure I understand this comment ...

> Tools/Scripts/webkitpy/w3c/test_importer.py:312
> +                        subpath = os.path.sep.join(subpath_parts[(index + 1):])

Why do we want to do this?

[Bem Jones-Bey]

Comment on attachment 206418 [details]
Patch

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

No worries about the delay, I was out on Friday and Monday myself. :-)

>> Tools/ChangeLog:18
>> +            no longer true, and I didn't see a good way to keep that behavior.
> 
> I'm not sure I understand this comment ...

There used to be a comment at the top of the file stating that it would fail if you try to try to import the full set of tests under contributors/*/submitted. I tried, and it works just fine after my changes, so that comment is no longer correct, so I removed it. I did not attempt to see what the failure mode was with the previous code. Does that make more sense?

> Tools/Scripts/webkitpy/w3c/test_importer.py:-59
> -    - For the time being, this script won't work if you try to import the full set of submitted
> -      tests under contributors/*/submitted. Since these are awaiting review, this is just a small
> -      control mechanism to enforce carefully selecting what non-approved tests are imported.
> -      It can obviously and easily be changed.
> -

This is the comment I'm referring to. The script does not have this behavior after my changes. (Though I did not check with the unmodified script to ensure that it had this behavior beforehand, I just trusted the comment)

>> Tools/Scripts/webkitpy/w3c/test_importer.py:312
>> +                        subpath = os.path.sep.join(subpath_parts[(index + 1):])
> 
> Why do we want to do this?

In order to have the behavior as documented in test_importer.py:

    - If the tests are submitted, they'll be brought in as LayoutTests/csswg/submitted and will also
      maintain their directory structure under that. For example, everything under
      <csswg_repo_root>/contributors/adobe/submitted is brought into submitted, mirroring its
      directory structure in the csswg repo

If that's not the desiered behavior, I'm happy to change it. (or did that mean it should end up in LayoutTests/csswg/submitted/contributors/adobe/submitted ?

[Dirk Pranke]

Comment on attachment 206418 [details]
Patch

Argh, I thought I had replied but apparently I never published the comments. Sorry!

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

>>> Tools/ChangeLog:18
>>> +            no longer true, and I didn't see a good way to keep that behavior.
>> 
>> I'm not sure I understand this comment ...
> 
> There used to be a comment at the top of the file stating that it would fail if you try to try to import the full set of tests under contributors/*/submitted. I tried, and it works just fine after my changes, so that comment is no longer correct, so I removed it. I did not attempt to see what the failure mode was with the previous code. Does that make more sense?

Yes.

>> Tools/Scripts/webkitpy/w3c/test_importer.py:-59
>> -
> 
> This is the comment I'm referring to. The script does not have this behavior after my changes. (Though I did not check with the unmodified script to ensure that it had this behavior beforehand, I just trusted the comment)

Ok.

>>> Tools/Scripts/webkitpy/w3c/test_importer.py:312
>>> +                        subpath = os.path.sep.join(subpath_parts[(index + 1):])
>> 
>> Why do we want to do this?
> 
> In order to have the behavior as documented in test_importer.py:
> 
>     - If the tests are submitted, they'll be brought in as LayoutTests/csswg/submitted and will also
>       maintain their directory structure under that. For example, everything under
>       <csswg_repo_root>/contributors/adobe/submitted is brought into submitted, mirroring its
>       directory structure in the csswg repo
> 
> If that's not the desiered behavior, I'm happy to change it. (or did that mean it should end up in LayoutTests/csswg/submitted/contributors/adobe/submitted ?

Ah, I think I had forgotten how the CSSWG's directories were laid out. So, when we're doing the full import, I expect us to have LayoutTests/csswg/contributors/adobe/submitted (i.e., the first "submitted" doesn't exist).

If you're doing a manual import somewhere else in the tree, you can pick the naming convention you'd like to follow, but as a user I would find it a bit confusing if we were only doing subtrees of the directory I specified on the command line.

I'm not entirely sure I understand your example, can you restate it with exact command line syntax and what files end up where? Ideally we'd have a test in test_importer_unittest.py that would feed it a command line and a populated MockFilesystem containing the files and show where we'd expect the files to be imported to, but I'm not quite sure if the test_importer.py script has all the right scaffolding to do that very easily, so if not I'll accept bug comments as samples :).

[Bem Jones-Bey]

Comment on attachment 206418 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/w3c/test_importer.py:312
>>>> +                        subpath = os.path.sep.join(subpath_parts[(index + 1):])
>>> 
>>> Why do we want to do this?
>> 
>> In order to have the behavior as documented in test_importer.py:
>> 
>>     - If the tests are submitted, they'll be brought in as LayoutTests/csswg/submitted and will also
>>       maintain their directory structure under that. For example, everything under
>>       <csswg_repo_root>/contributors/adobe/submitted is brought into submitted, mirroring its
>>       directory structure in the csswg repo
>> 
>> If that's not the desiered behavior, I'm happy to change it. (or did that mean it should end up in LayoutTests/csswg/submitted/contributors/adobe/submitted ?
> 
> Ah, I think I had forgotten how the CSSWG's directories were laid out. So, when we're doing the full import, I expect us to have LayoutTests/csswg/contributors/adobe/submitted (i.e., the first "submitted" doesn't exist).
> 
> If you're doing a manual import somewhere else in the tree, you can pick the naming convention you'd like to follow, but as a user I would find it a bit confusing if we were only doing subtrees of the directory I specified on the command line.
> 
> I'm not entirely sure I understand your example, can you restate it with exact command line syntax and what files end up where? Ideally we'd have a test in test_importer_unittest.py that would feed it a command line and a populated MockFilesystem containing the files and show where we'd expect the files to be imported to, but I'm not quite sure if the test_importer.py script has all the right scaffolding to do that very easily, so if not I'll accept bug comments as samples :).

LayoutTests/csswg/contributors/adobe/submitted makes sense to me, that's what I would have done if not for that comment in test_importer.py that I mentioned. I'll change it so that the import has that behavior and upload a new patch.

I'm not sure what example you are referring to, but when I import my tests, I'm doing something like:
./Tools/Scripts/import-w3c-tests -d csswg ~/Code/csswg-test/contributors/adobe/submitted/shapes/shape-outside ~/Code/csswg-test

And it's importing into:
LayoutTests/csswg/submitted/shapes/shape-outside

It sounds like it need to update it to import into:
LayoutTests/csswg/contributors/adobe/submitted/shapes/shape-outside

Which is just fine by me, I didn't do that only because I was trying to keep with the documented behavior. I will make the change and update the documentation unless you object.

[Dirk Pranke]

(In reply to comment #10)
> It sounds like it need to update it to import into:
> LayoutTests/csswg/contributors/adobe/submitted/shapes/shape-outside

Yup, that's exactly what I had in mind.

[Bem Jones-Bey]

Created attachment 207880 [details]
Patch

Update to preserve contributor dir paths

[WebKit Commit Bot]

Comment on attachment 207880 [details]
Patch

Clearing flags on attachment: 207880

Committed r153545: <http://trac.webkit.org/changeset/153545>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.