Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Thu, Apr 29, 2010 at 9:44 AM, Alexey Proskuryakov a...@webkit.org wrote: On 26.04.2010, at 22:06, Adam Barth wrote: If you make changes to CodeGenerator*.pm, please test your change using the following command: ~/git/webkit$ ./WebKitTools/Scripts/run-bindings-tests I don't understand the utility of such testing. When one edits CodeGenerator*.pm, that's generally to change its output, so run-bindings-tests tests will of course fail. It's great to test end-to-end behavior, and unit tests can also also useful sometimes, but why test that source code stays byte to byte identical? I should say that there's also a class of changes to CodeGenerator*.pm where we don't want to change its output at all. For example, removing some of the ridiculously deeply nested if statements. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On 29.04.2010, at 10:27, Jeremy Orlow wrote: It's great to test end-to-end behavior, and unit tests can also also useful sometimes, but why test that source code stays byte to byte identical? When you make a change to the code generator, you should make a corresponding change to the generated test code. This allows the reviewer to see how the change in CodeGenerator*.pm affects the generated code and documents the change in SVN. The hope is that it'll be obvious if your change is going to have unintended consequences on the generated code. We've found this useful when working on the V8 bindings. For these goals to be achieved, we'd need to check all generated code, not just one custom test file for each language. It would be great to have a tool that generates a diff of derived sources for inspection, but making it into a test for everyone to maintain feels like unnecessary burden. I certainly would feel bad about having to maintain a test that verifies source file content instead of behavior. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On 29.04.2010, at 10:30, Adam Barth wrote: Yeah, without seeing the consequence on the generated code, it can be very difficult to review changes to the code generator. Yes, that's a useful goal. Since an old version of code generating scripts is always in .svn directory (or its git equivalent), it should be possible to generate before and after versions of DerivedSources directory, and diff these. Such a tool would be useful and exciting. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
ext Alexey Proskuryakov wrote: On 29.04.2010, at 10:30, Adam Barth wrote: Yeah, without seeing the consequence on the generated code, it can be very difficult to review changes to the code generator. Yes, that's a useful goal. Since an old version of code generating scripts is always in .svn directory (or its git equivalent), it should be possible to generate before and after versions of DerivedSources directory, and diff these. Such a tool would be useful and exciting. Definitely. The generator for Qt Jambi (Qt bindings for Java, http://qt.gitorious.org/qt-jambi) has a --diff option that produces a colorized diff against the output of the previous run. It's a great, simple way of building confidence that the generator change only had the impact you intended (no guarantees of course, due to platform differences, feature permutations etc.). In my experience such a feature is like turning on the lights (or cranking up the generator, to use a direct analogy). Kent ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Thu, Apr 29, 2010 at 10:54 AM, Adam Barth aba...@webkit.org wrote: On Thu, Apr 29, 2010 at 10:39 AM, Alexey Proskuryakov a...@webkit.org wrote: On 29.04.2010, at 10:27, Jeremy Orlow wrote: It's great to test end-to-end behavior, and unit tests can also also useful sometimes, but why test that source code stays byte to byte identical? When you make a change to the code generator, you should make a corresponding change to the generated test code. This allows the reviewer to see how the change in CodeGenerator*.pm affects the generated code and documents the change in SVN. The hope is that it'll be obvious if your change is going to have unintended consequences on the generated code. We've found this useful when working on the V8 bindings. For these goals to be achieved, we'd need to check all generated code, not just one custom test file for each language. The idea is that the test IDL file exercises the features of the code generator. Sure, it's not 100% coverage, but it's much better not testing it at all. It would be great to have a tool that generates a diff of derived sources for inspection, but making it into a test for everyone to maintain feels like unnecessary burden. I certainly would feel bad about having to maintain a test that verifies source file content instead of behavior. You should feel free to develop a better testing harness. This one certainly isn't best conceivable tool, but it's better than what we had previously, which was essentially the C++ compiler. The maintenance is super easy. I've been doing a lot of development work on the code generator in the past few days, and it amounts to typing a single command: ./WebKitTools/Scripts/run-bindings-tests --reset-results The harness has been super useful in working on the code generator because the tests run in a few seconds. That lets me iterate on the script much more quickly compared to rebuilding the world every time I want to try a tweak. As an additional datapoint on maintenance, it looks like you've touched CodeGeneratorJS.pm twice in the past year and neither of those change would have resulted in a large diff to the expected test results. By contrast, I've made five changes in the past two days and plan to made dozens more changes shortly to reduce the amount of custom bindings code we have in the tree, which is a serious maintenance problem because it is full of subtle, quirky behavior. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Apr 29, 2010, at 10:54 AM, Adam Barth wrote: It would be great to have a tool that generates a diff of derived sources for inspection, but making it into a test for everyone to maintain feels like unnecessary burden. I certainly would feel bad about having to maintain a test that verifies source file content instead of behavior. You should feel free to develop a better testing harness. This one certainly isn't best conceivable tool, but it's better than what we had previously, which was essentially the C++ compiler. The maintenance is super easy. I've been doing a lot of development work on the code generator in the past few days, and it amounts to typing a single command: ./WebKitTools/Scripts/run-bindings-tests --reset-results The harness has been super useful in working on the code generator because the tests run in a few seconds. That lets me iterate on the script much more quickly compared to rebuilding the world every time I want to try a tweak. It also strikes me as odd to do testing by doing exact comparison of the generated source. But I can also see side benefits. I think the real issue here may be one of naming. If the use model is that you fully regenerate the results every time you make a change to the bindings generator, then it's not really a regression test. The purpose is not to catch unintentional changes but rather to be able to observe changes to generated code, and new kinds of generated code, while working on a change and when reviewing code. Perhaps the tool should have a name that reflects that, instead of implying that the purpose is to catch bugs accidentally introduced by changes. It doesn't seem like an efficient or effective way to do the latter. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On 29.04.2010, at 10:54, Adam Barth wrote: You should feel free to develop a better testing harness. Since you're proposing run-bindings-tests as a required tool for everyone to use, I don't think that this resolves my concerns. The maintenance is super easy. I've been doing a lot of development work on the code generator in the past few days, and it amounts to typing a single command: ./WebKitTools/Scripts/run-bindings-tests --reset-results So, isn't once supposed to extend the tests when e.g. adding new flags? - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On 29.04.2010, at 11:17, Yaar Schnitman wrote: I've been using the tool for a couple of patches in V8. It really boost the development cycle, helps reviewers understanding what a cryptic perl block of code actually does, and side effects are easy to find. Once you start using it, its becoming hard to work without it. Give it a try! 'm thinking about how this tool could have helped with the CodeGenerator changes I made in the past, and it seems that it wouldn't have detected any changes, and could require me to find creative ways to test the new behavior. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Thu, Apr 29, 2010 at 11:39 AM, Alexey Proskuryakov a...@webkit.org wrote: On 29.04.2010, at 11:17, Yaar Schnitman wrote: I've been using the tool for a couple of patches in V8. It really boost the development cycle, helps reviewers understanding what a cryptic perl block of code actually does, and side effects are easy to find. Once you start using it, its becoming hard to work without it. Give it a try! 'm thinking about how this tool could have helped with the CodeGenerator changes I made in the past, and it seems that it wouldn't have detected any changes, and could require me to find creative ways to test the new behavior. I don't really follow the what the maintenance overhead is. How does this actually cause you more than a trivial amount of extra work? Maybe a specific example would help. Isn't this just like having a layout test with expected results? It's a small isolated test instead of testing everything. That seems like a good thing. More importantly, it lets you be sure that every feature of the code generator has some testing. In the real IDLs, a feature might stop getting used temporarily and then changes to the code generator would not be readily apparent. Ojan P.S. Sorry for the double-post some of you got. Sent from the wrong email address at first. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
As a concrete example, I found this test setup helpful for this patch: http://trac.webkit.org/changeset/58345. A nice side effect was that it revealed a bug in CodeGeneratorGObject.pm and let me fix it without having to set up build setup for whatever it is that uses the GObject bindings. I agree that golden file testing is a very high-maintenance fragile test method, but it's better than nothing. If this framework didn't exist then I would have likely made the change and relied on spot checking and our existing automated tests to catch any regressions which is less than ideal. - James On Thu, Apr 29, 2010 at 11:44 AM, Ojan Vafai o...@chromium.org wrote: On Thu, Apr 29, 2010 at 11:39 AM, Alexey Proskuryakov a...@webkit.orgwrote: On 29.04.2010, at 11:17, Yaar Schnitman wrote: I've been using the tool for a couple of patches in V8. It really boost the development cycle, helps reviewers understanding what a cryptic perl block of code actually does, and side effects are easy to find. Once you start using it, its becoming hard to work without it. Give it a try! 'm thinking about how this tool could have helped with the CodeGenerator changes I made in the past, and it seems that it wouldn't have detected any changes, and could require me to find creative ways to test the new behavior. I don't really follow the what the maintenance overhead is. How does this actually cause you more than a trivial amount of extra work? Maybe a specific example would help. Isn't this just like having a layout test with expected results? It's a small isolated test instead of testing everything. That seems like a good thing. More importantly, it lets you be sure that every feature of the code generator has some testing. In the real IDLs, a feature might stop getting used temporarily and then changes to the code generator would not be readily apparent. Ojan P.S. Sorry for the double-post some of you got. Sent from the wrong email address at first. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
Just curious, would it be less maintenance if the test run was integrated with run-webkit-tests?/Is the concern about having lots of different tests harness to run to verify a change? dave On Thu, Apr 29, 2010 at 12:11 PM, James Robinson jam...@google.com wrote: As a concrete example, I found this test setup helpful for this patch: http://trac.webkit.org/changeset/58345. A nice side effect was that it revealed a bug in CodeGeneratorGObject.pm and let me fix it without having to set up build setup for whatever it is that uses the GObject bindings. I agree that golden file testing is a very high-maintenance fragile test method, but it's better than nothing. If this framework didn't exist then I would have likely made the change and relied on spot checking and our existing automated tests to catch any regressions which is less than ideal. - James On Thu, Apr 29, 2010 at 11:44 AM, Ojan Vafai o...@chromium.org wrote: On Thu, Apr 29, 2010 at 11:39 AM, Alexey Proskuryakov a...@webkit.orgwrote: On 29.04.2010, at 11:17, Yaar Schnitman wrote: I've been using the tool for a couple of patches in V8. It really boost the development cycle, helps reviewers understanding what a cryptic perl block of code actually does, and side effects are easy to find. Once you start using it, its becoming hard to work without it. Give it a try! 'm thinking about how this tool could have helped with the CodeGenerator changes I made in the past, and it seems that it wouldn't have detected any changes, and could require me to find creative ways to test the new behavior. I don't really follow the what the maintenance overhead is. How does this actually cause you more than a trivial amount of extra work? Maybe a specific example would help. Isn't this just like having a layout test with expected results? It's a small isolated test instead of testing everything. That seems like a good thing. More importantly, it lets you be sure that every feature of the code generator has some testing. In the real IDLs, a feature might stop getting used temporarily and then changes to the code generator would not be readily apparent. Ojan P.S. Sorry for the double-post some of you got. Sent from the wrong email address at first. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
IMHO, run-webkit-tests should run all the various webkit testing scripts and we should have a run-layout-tests script that does what run-webkit-tests does today. I'd also settle for a run-tests scripts that was the ASAD testing script. Adam On Thu, Apr 29, 2010 at 12:16 PM, David Levin le...@chromium.org wrote: Just curious, would it be less maintenance if the test run was integrated with run-webkit-tests?/Is the concern about having lots of different tests harness to run to verify a change? dave On Thu, Apr 29, 2010 at 12:11 PM, James Robinson jam...@google.com wrote: As a concrete example, I found this test setup helpful for this patch: http://trac.webkit.org/changeset/58345. A nice side effect was that it revealed a bug in CodeGeneratorGObject.pm and let me fix it without having to set up build setup for whatever it is that uses the GObject bindings. I agree that golden file testing is a very high-maintenance fragile test method, but it's better than nothing. If this framework didn't exist then I would have likely made the change and relied on spot checking and our existing automated tests to catch any regressions which is less than ideal. - James On Thu, Apr 29, 2010 at 11:44 AM, Ojan Vafai o...@chromium.org wrote: On Thu, Apr 29, 2010 at 11:39 AM, Alexey Proskuryakov a...@webkit.org wrote: On 29.04.2010, at 11:17, Yaar Schnitman wrote: I've been using the tool for a couple of patches in V8. It really boost the development cycle, helps reviewers understanding what a cryptic perl block of code actually does, and side effects are easy to find. Once you start using it, its becoming hard to work without it. Give it a try! 'm thinking about how this tool could have helped with the CodeGenerator changes I made in the past, and it seems that it wouldn't have detected any changes, and could require me to find creative ways to test the new behavior. I don't really follow the what the maintenance overhead is. How does this actually cause you more than a trivial amount of extra work? Maybe a specific example would help. Isn't this just like having a layout test with expected results? It's a small isolated test instead of testing everything. That seems like a good thing. More importantly, it lets you be sure that every feature of the code generator has some testing. In the real IDLs, a feature might stop getting used temporarily and then changes to the code generator would not be readily apparent. Ojan P.S. Sorry for the double-post some of you got. Sent from the wrong email address at first. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Apr 29, 2010, at 11:38 AM, Ojan Vafai wrote: I don't really follow the argument against supporting this test. What is the maintenance overhead? Isn't this just like having a layout test with expected results? It's a small isolated test instead of testing everything. That seems like a good thing. More importantly, it lets you be sure that every feature of the code generator has some testing. In the real IDLs, a feature might stop getting used temporarily and then changes to the code generator would not be readily apparent. I don't think your comments above are responsive to my paragraph below: On Thu, Apr 29, 2010 at 11:05 AM, Maciej Stachowiak m...@apple.com wrote: It also strikes me as odd to do testing by doing exact comparison of the generated source. But I can also see side benefits. I think the real issue here may be one of naming. If the use model is that you fully regenerate the results every time you make a change to the bindings generator, then it's not really a regression test. The purpose is not to catch unintentional changes but rather to be able to observe changes to generated code, and new kinds of generated code, while working on a change and when reviewing code. Perhaps the tool should have a name that reflects that, instead of implying that the purpose is to catch bugs accidentally introduced by changes. It doesn't seem like an efficient or effective way to do the latter. To repeat, I think this is a useful tool, but not necessarily as a test. The mode of operation is that when you run this test, if the generated bindings for the text IDL file change in any way, it reports a failure. Then you must manually take the step of regenerating the golden master file. It doesn't seem like the failure report itself will result in a decision. It doesn't provide interesting information until you regenerate the test file and look at the diffs, except in the highly unusual case of changing the output of the codegen script unintentially. Most changes to the codegen script are to intentionally change the output. It seems to me a better model would be to regenerate the bindings test file automatically as part of the build. Then the changes can still be reviewed by you, or as part of a diff, but there would be no extra manual steps involved. And people making behaviorally transparent changes to codegen output would perhaps feel a little less burdened. That makes more sense to me than treating it as a regression test. It also seems like this model would still have all the benefits you cite above. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Thu, Apr 29, 2010 at 12:43 PM, Maciej Stachowiak m...@apple.com wrote: To repeat, I think this is a useful tool, but not necessarily as a test. The mode of operation is that when you run this test, if the generated bindings for the text IDL file change in any way, it reports a failure. Then you must manually take the step of regenerating the golden master file. It doesn't seem like the failure report itself will result in a decision. It doesn't provide interesting information until you regenerate the test file and look at the diffs, except in the highly unusual case of changing the output of the codegen script unintentially. Most changes to the codegen script are to intentionally change the output. The failure report shows you the diff. In hacking on CodeGeneratorJS, I often run the script many times to see how my changes are affecting the generated code, much in the same way I would run all the LayoutTests constantly if they took 5s to run. It seems to me a better model would be to regenerate the bindings test file automatically as part of the build. Then the changes can still be reviewed by you, or as part of a diff, but there would be no extra manual steps involved. And people making behaviorally transparent changes to codegen output would perhaps feel a little less burdened. That sounds like a good improvement. I think it would be fine to regenerate the output as part of the build. However, I think we should still preserve the ability to run the script along it test mode because that's about three orders of magnitude faster than performing a build after touching CodeGeneratorJS. That makes more sense to me than treating it as a regression test. It also seems like this model would still have all the benefits you cite above. What I hear from this conversation is the following: 1) A bunch of people who've used the tool saying that they've found it useful. 2) A bunch of people who haven't used the tool suggesting improvements. This tool impacts the handful of people who work on CodeGeneratorJS.pm. Everyone else in the project can safely ignore it. I'm all for improvements, and I invite anyone interested to either improve the tool or write a new tool that does the job better. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
I see run-bindings-tests is primary a productivity tool: Faster development, easy debugging, better reviews. But it only works if the golden copies don't fall into disrepair = Hence the need to bake this tool into the general testing framework (run-webkit-tests, pre-submit checks, etc.). It will become a useful (but not perfect) regression tool as a side effect of that. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Apr 29, 2010, at 1:05 PM, Adam Barth wrote: On Thu, Apr 29, 2010 at 12:43 PM, Maciej Stachowiak m...@apple.com wrote: It seems to me a better model would be to regenerate the bindings test file automatically as part of the build. Then the changes can still be reviewed by you, or as part of a diff, but there would be no extra manual steps involved. And people making behaviorally transparent changes to codegen output would perhaps feel a little less burdened. That sounds like a good improvement. I think it would be fine to regenerate the output as part of the build. However, I think we should still preserve the ability to run the script along it test mode because that's about three orders of magnitude faster than performing a build after touching CodeGeneratorJS. Alexey (or others who don't like the new tests), do you think this change would address your concerns? On Apr 29, 2010, at 1:05 PM, Adam Barth wrote: What I hear from this conversation is the following: 1) A bunch of people who've used the tool saying that they've found it useful. 2) A bunch of people who haven't used the tool suggesting improvements. This tool impacts the handful of people who work on CodeGeneratorJS.pm. Everyone else in the project can safely ignore it. I'm all for improvements, and I invite anyone interested to either improve the tool or write a new tool that does the job better. If everyone has to use the tool for the tool to be useful, then ideally we want a system where the people who change the bindings frequently mostly buy into. Here is the list of people with more than 5 all-time commits in the WebCore/bindings/scripts directory. Ideally I'd like to hear from more of these what they think would be helpful and not burdensome. 59 wei...@apple.com 46 e...@webkit.org 35 da...@apple.com 32 jap...@chromium.org 29 oli...@apple.com 26 gga...@apple.com 26 dglaz...@chromium.org 16 aba...@webkit.org 14 zimmerm...@webkit.org 12 a...@webkit.org 10 aro...@apple.com 8 le...@chromium.org 7 m...@apple.com 7 da...@chromium.org 6 timo...@apple.com 6 s...@chromium.org 6 jia...@chromium.org 6 ddkil...@apple.com 6 cwzwar...@webkit.org Here is the command anyone can run to see the full list (assuming you have an SVN checkout): $ svn log WebCore/bindings/scripts | grep '|.*@' | sed -e 's/^[^|]* |// g; s/ | .*$//g;' | sort | uniq -c | sort -rn The long tail of people who have made only a few bindings changes is rather large, so I suspect this tool affects more than a handful people, if it becomes a mandatory part of the process. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On 29.04.2010, at 13:38, Maciej Stachowiak wrote: Alexey (or others who don't like the new tests), do you think this change would address your concerns? It would definitely be better if changes weren't detected as failures. Changing a checked out file as part of build seems rather magical though. I'm not sure if everyone agrees that making a diff of actual generated files would be better than looking at a single custom test. That's something I wished for many times, but it calls for a different workflow. And of course, that's not something that is currently implemented. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
Since I'm in the bindings hall of shame, I guess I'm supposed to reply. ;) The twice that I've used it, it was very helpful. The few reviews I've done of Adam's it was much better than what we had before. However, I agree something better could be built. I'm just not sure what better looks like yet. I expect when Adam finishes getting rid of custom bindings code he'll have a better idea. :) I'm surprised this thread so much attention. -eric On Thu, Apr 29, 2010 at 1:38 PM, Maciej Stachowiak m...@apple.com wrote: On Apr 29, 2010, at 1:05 PM, Adam Barth wrote: On Thu, Apr 29, 2010 at 12:43 PM, Maciej Stachowiak m...@apple.com wrote: It seems to me a better model would be to regenerate the bindings test file automatically as part of the build. Then the changes can still be reviewed by you, or as part of a diff, but there would be no extra manual steps involved. And people making behaviorally transparent changes to codegen output would perhaps feel a little less burdened. That sounds like a good improvement. I think it would be fine to regenerate the output as part of the build. However, I think we should still preserve the ability to run the script along it test mode because that's about three orders of magnitude faster than performing a build after touching CodeGeneratorJS. Alexey (or others who don't like the new tests), do you think this change would address your concerns? On Apr 29, 2010, at 1:05 PM, Adam Barth wrote: What I hear from this conversation is the following: 1) A bunch of people who've used the tool saying that they've found it useful. 2) A bunch of people who haven't used the tool suggesting improvements. This tool impacts the handful of people who work on CodeGeneratorJS.pm. Everyone else in the project can safely ignore it. I'm all for improvements, and I invite anyone interested to either improve the tool or write a new tool that does the job better. If everyone has to use the tool for the tool to be useful, then ideally we want a system where the people who change the bindings frequently mostly buy into. Here is the list of people with more than 5 all-time commits in the WebCore/bindings/scripts directory. Ideally I'd like to hear from more of these what they think would be helpful and not burdensome. 59 wei...@apple.com 46 e...@webkit.org 35 da...@apple.com 32 jap...@chromium.org 29 oli...@apple.com 26 gga...@apple.com 26 dglaz...@chromium.org 16 aba...@webkit.org 14 zimmerm...@webkit.org 12 �...@webkit.org 10 aro...@apple.com 8 le...@chromium.org 7 ...@apple.com 7 da...@chromium.org 6 timo...@apple.com 6 s...@chromium.org 6 jia...@chromium.org 6 ddkil...@apple.com 6 cwzwar...@webkit.org Here is the command anyone can run to see the full list (assuming you have an SVN checkout): $ svn log WebCore/bindings/scripts | grep '|.*@' | sed -e 's/^[^|]* |//g; s/ | .*$//g;' | sort | uniq -c | sort -rn The long tail of people who have made only a few bindings changes is rather large, so I suspect this tool affects more than a handful people, if it becomes a mandatory part of the process. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Testing changes to CodeGenerator*.pm
On Thu, Apr 29, 2010 at 9:05 PM, Adam Barth aba...@webkit.org wrote: On Thu, Apr 29, 2010 at 12:43 PM, Maciej Stachowiak m...@apple.com wrote: It seems to me a better model would be to regenerate the bindings test file automatically as part of the build. Then the changes can still be reviewed by you, or as part of a diff, but there would be no extra manual steps involved. And people making behaviorally transparent changes to codegen output would perhaps feel a little less burdened. That sounds like a good improvement. I think it would be fine to regenerate the output as part of the build. However, I think we should still preserve the ability to run the script along it test mode because that's about three orders of magnitude faster than performing a build after touching CodeGeneratorJS. From reading this thread, this seems like the best solution and most easy to accomplish. We already have the standalone script, so all we need to do is hack it into the build. Isn't there some makefile that generates the derived sources for at least most of the platforms? Couldn't we just add it to that? People who don't care or value it could continue with their current work-flows unaffected. And those that it helps can continue with their workflows as well, as far as I can tell. And, optionally, if someone would like to create more robust testing frameworks in the future could. Am I totally missing something? I feel like I must since this discussion has continued on for a while beyond this point. J ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Testing changes to CodeGenerator*.pm
People of webkit-dev: We now have a testing harness in place for CodeGeneratorJS.pm and friends. If you make changes to CodeGenerator*.pm, please test your change using the following command: ~/git/webkit$ ./WebKitTools/Scripts/run-bindings-tests If you add a new feature to CodeGenerator*.pm, please exercise that feature by adding a declaration in our test IDL file: http://trac.webkit.org/browser/trunk/WebCore/bindings/scripts/test/TestObj.idl If your patch changes the expected results, you can generate new results as follows: ~/git/webkit$ ./WebKitTools/Scripts/run-bindings-tests --overwrite That way folks can review the change to the expected results along side your change to the generator. Happy code generation hacking! Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
