Update location of w3c shapes tests to reflect new import location
Created attachment 205647 [details] Patch Update import location for new import script
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 .
(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?
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.
Created attachment 206418 [details] Patch First attempt at enabling partial imports to a special directory.
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.
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?
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 ?
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 :).
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.
(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.
Created attachment 207880 [details] Patch Update to preserve contributor dir paths
Comment on attachment 207880 [details] Patch Clearing flags on attachment: 207880 Committed r153545: <http://trac.webkit.org/changeset/153545>
All reviewed patches have been landed. Closing bug.