63479 2011-06-27 14:03:11 -0700 webkitpy unit tests should have more descriptive names than just "Test" 2011-06-27 14:59:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 abarth abarth dpranke eric oldest_to_newest 428266 0 abarth 2011-06-27 14:03:11 -0700 webkitpy unit tests should have more descriptive names than just "Test" 428268 1 98780 abarth 2011-06-27 14:04:19 -0700 Created attachment 98780 Patch 428275 2 98780 dpranke 2011-06-27 14:09:40 -0700 Comment on attachment 98780 Patch The changes look fine, but aren't the names scoped to the module? I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done. Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so. 428286 3 abarth 2011-06-27 14:16:54 -0700 > The changes look fine, but aren't the names scoped to the module? It depends on how they're imported. As it happens, we import them with their fully qualified names, so there's no conflict. However, if we imported them all into one flat namespace, we'd get in trouble. > I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done. Yeah, this mostly just code cleanliness patch. "Test" is just a generic name. > Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so. My view is that this is mostly a matter of consistency. Currently some of the unit tests files have this clause at the bottom and some don't, which makes it unpredictable whether running the file directly will do anything sensible. Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests? Currently, you need to pass the fully qualified class name, which is less than ideal. Something like the gtest_filter is something we could emulate. 428289 4 98780 abarth 2011-06-27 14:18:35 -0700 Comment on attachment 98780 Patch Clearing flags on attachment: 98780 Committed r89852: <http://trac.webkit.org/changeset/89852> 428290 5 abarth 2011-06-27 14:18:38 -0700 All reviewed patches have been landed. Closing bug. 428300 6 dpranke 2011-06-27 14:23:20 -0700 (In reply to comment #3) > > The changes look fine, but aren't the names scoped to the module? > > It depends on how they're imported. As it happens, we import them with their fully qualified names, so there's no conflict. However, if we imported them all into one flat namespace, we'd get in trouble. > > > I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done. > > Yeah, this mostly just code cleanliness patch. "Test" is just a generic name. > Yup, I agree, which is why I approved it :). > > Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so. > > My view is that this is mostly a matter of consistency. Currently some of the unit tests files have this clause at the bottom and some don't, which makes it unpredictable whether running the file directly will do anything sensible. > I also agree with this, and have made it a practice in the past to add them where they were missing when I was otherwise changing a file. Of course, if others are practicing the opposite strategy, this can only end in unhappiness. > Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests? Currently, you need to pass the fully qualified class name, which is less than ideal. Something like the gtest_filter is something we could emulate. Possibly; it would certainly be better. In my personal work habits I usually have a shell open and in the directory containing the test files (or maybe one dir up), so "python foo_unittest.py" is easier than "test-webkitpy path-to-test-file" would be, usually (if test-webkitpy was smart enough to figure things out from the current working directory and a relative name, that would be fine). E.g. 'test-webkitpy foo' could look for foo_*test.py files in the current working dir. Also running the tests directly integrates nicely with the python coverage package; it's been a while since I tried test-webkitpy to see if that also did, but I'd want to preserve that if possible. 428304 7 abarth 2011-06-27 14:26:03 -0700 Another issue is that test_webkitpy sets up the environment for the tests. For example, it configures a bunch of logging settings. When you run the tests individually, you're running them in a different environment. This all just seems like an elaborate workaround for the fact that the tests are slow. If test-webkitpy ran in zero seconds, there would be no reason not to run the whole suite. 428308 8 eric 2011-06-27 14:27:24 -0700 (In reply to comment #6) > > Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests? Currently, you need to pass the fully qualified class name, which is less than ideal. Something like the gtest_filter is something we could emulate. > > Possibly; it would certainly be better. In my personal work habits I usually have a shell open and in the directory containing the test files (or maybe one dir up), so "python foo_unittest.py" is easier than "test-webkitpy path-to-test-file" would be, usually (if test-webkitpy was smart enough to figure things out from the current working directory and a relative name, that would be fine). E.g. 'test-webkitpy foo' could look for foo_*test.py files in the current working dir. > > Also running the tests directly integrates nicely with the python coverage package; it's been a while since I tried test-webkitpy to see if that also did, but I'd want to preserve that if possible. Interesting, I was not aware of the coverage usecase. Clearly running a test directly with python test.py is differnet from running test-webkitpy. Both in import constraints as well as logging (there are probably other ways too). So I've been less excited about supporting that mode. But perhaps there are arguments like the coverage argument which better support this individual mode. I'd like to either: - teach webkipy to run them all individually, like you're proposing doing, and auto-add some sort of main - remove the ability to run them individually, and instead add all the individual-run features to test-webkitpy Having two ways to run the tests seems only to end in sadness (like how it's possible to run them under multiple versions of python does today). 428340 9 dpranke 2011-06-27 14:59:46 -0700 (In reply to comment #7) > Another issue is that test_webkitpy sets up the environment for the tests. For example, it configures a bunch of logging settings. When you run the tests individually, you're running them in a different environment. > This is true, although (a) it rarely seems to matter for me and (b) the environment changes that test-webkitpy does seems to be largely done to work around tests that log more than they should (or log it less controllably). The separate bug that has been open forever about how test-webkitpy configures logging is the right place to debate that, probably. > This all just seems like an elaborate workaround for the fact that the tests are slow. If test-webkitpy ran in zero seconds, there would be no reason not to run the whole suite. I don't think this is quite true; running the tests (or subsets of tests individually) is useful for debugging why things are failing, and is useful for figuring out test coverage, as I mentioned. But, assuming you can get that functionality easily enough in test-webkitpy (and I don't have any reason to think we couldn't), these differences are just cosmetic. (In reply to comment #8) > But perhaps there are arguments like the coverage argument which better support this individual mode. > > I'd like to either: > > - teach webkipy to run them all individually, like you're proposing doing, and auto-add some sort of main > - remove the ability to run them individually, and instead add all the individual-run features to test-webkitpy We definitely need to keep the ability to run individual tests. I don't care that much what tool does it, but there needs to be a tool that can run a list of specific test methods and even test modules. > Having two ways to run the tests seems only to end in sadness (like how it's possible to run them under multiple versions of python does today). I don't think the analogy is quite the same, but I agree that we should aim for one tool that meets all the needs (or as many as possible). Feel free to add the features to test-webkitpy and then delete all of these lines :) 98780 2011-06-27 14:04:19 -0700 2011-06-27 14:18:34 -0700 Patch bug-63479-20110627140418.patch text/plain 3399 abarth SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDg5ODUwKQorKysgVG9vbHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTcgQEAKKzIwMTEtMDYtMjcgIEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICB3ZWJraXRweSB1 bml0IHRlc3RzIHNob3VsZCBoYXZlIG1vcmUgZGVzY3JpcHRpdmUgbmFtZXMgdGhhbiBqdXN0ICJU ZXN0IgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjM0 NzkKKworICAgICAgICBMdWNraWx5IHdlIGtlcHQgdGhlc2UgY2xhc3NlcyBpbiBzZXBhcmF0ZSBu YW1lc3BhY2VzIGluIHRoZSBoYXJuZXNzIHNvCisgICAgICAgIHdlIHdlcmUgYWN0dWFsbHkgcnVu bmluZyB0aGVtIGFsbCBldmVuIHRob3VnaCB0aGV5IGhhZCB0aGUgc2FtZSBuYW1lLgorCisgICAg ICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL3N0YWNrX3V0aWxzX3VuaXR0ZXN0 LnB5OgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL2xheW91dF9wYWNr YWdlL3Rlc3RfZmFpbHVyZXNfdW5pdHRlc3QucHk6CisgICAgICAgICogU2NyaXB0cy93ZWJraXRw eS9sYXlvdXRfdGVzdHMvbGF5b3V0X3BhY2thZ2UvdGVzdF9yZXN1bHRzX3VuaXR0ZXN0LnB5Ogor CiAyMDExLTA2LTI3ICBHcmVnIFNpbW9uICA8Z3JlZ3NpbW9uQGNocm9taXVtLm9yZz4KIAogICAg ICAgICBObyByZXZpZXcgbmVjZXNzYXJ5LgpJbmRleDogVG9vbHMvU2NyaXB0cy93ZWJraXRweS9j b21tb24vc3lzdGVtL3N0YWNrX3V0aWxzX3VuaXR0ZXN0LnB5Cj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xz L1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS9zdGFja191dGlsc191bml0dGVzdC5weQko cmV2aXNpb24gODk4NDgpCisrKyBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0v c3RhY2tfdXRpbHNfdW5pdHRlc3QucHkJKHdvcmtpbmcgY29weSkKQEAgLTM4LDcgKzM4LDcgQEAg ZGVmIGN1cnJlbnRfdGhyZWFkX2lkKCk6CiAgICAgcmV0dXJuIHRocmVhZF9pZAogCiAKLWNsYXNz IFRlc3QodW5pdHRlc3QuVGVzdENhc2UpOgorY2xhc3MgU3RhY2tVdGlsc1Rlc3QodW5pdHRlc3Qu VGVzdENhc2UpOgogICAgIGRlZiB0ZXN0X2ZpbmRfdGhyZWFkX3N0YWNrX2ZvdW5kKHNlbGYpOgog ICAgICAgICB0aHJlYWRfaWQgPSBjdXJyZW50X3RocmVhZF9pZCgpCiAgICAgICAgIGZvdW5kX3N0 YWNrID0gc3RhY2tfdXRpbHMuX2ZpbmRfdGhyZWFkX3N0YWNrKHRocmVhZF9pZCkKQEAgLTcwLDcg KzcwLDMgQEAgY2xhc3MgVGVzdCh1bml0dGVzdC5UZXN0Q2FzZSk6CiAgICAgICAgIGV4Y2VwdDoK ICAgICAgICAgICAgIHN0YWNrX3V0aWxzLmxvZ190cmFjZWJhY2sobG9nZ2VyLCBzeXMuZXhjX2lu Zm8oKVsyXSkKICAgICAgICAgc2VsZi5hc3NlcnRUcnVlKG1zZ3MpCi0KLQotaWYgX19uYW1lX18g PT0gJ19fbWFpbl9fJzoKLSAgICB1bml0dGVzdC5tYWluKCkKSW5kZXg6IFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvbGF5b3V0X3Rlc3RzL2xheW91dF9wYWNrYWdlL3Rlc3RfZmFpbHVyZXNfdW5pdHRl c3QucHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv bGF5b3V0X3BhY2thZ2UvdGVzdF9mYWlsdXJlc191bml0dGVzdC5weQkocmV2aXNpb24gODk4NDgp CisrKyBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9sYXlvdXRfcGFja2FnZS90 ZXN0X2ZhaWx1cmVzX3VuaXR0ZXN0LnB5CSh3b3JraW5nIGNvcHkpCkBAIC0zMyw3ICszMyw3IEBA IGltcG9ydCB1bml0dGVzdAogZnJvbSB3ZWJraXRweS5sYXlvdXRfdGVzdHMubGF5b3V0X3BhY2th Z2UudGVzdF9mYWlsdXJlcyBpbXBvcnQgKgogCiAKLWNsYXNzIFRlc3QodW5pdHRlc3QuVGVzdENh c2UpOgorY2xhc3MgVGVzdEZhaWx1cmVzZXN0KHVuaXR0ZXN0LlRlc3RDYXNlKToKICAgICBkZWYg YXNzZXJ0X2xvYWRzKHNlbGYsIGNscyk6CiAgICAgICAgIGZhaWx1cmVfb2JqID0gY2xzKCkKICAg ICAgICAgcyA9IGZhaWx1cmVfb2JqLmR1bXBzKCkKQEAgLTgzLDYgKzgzLDMgQEAgY2xhc3MgVGVz dCh1bml0dGVzdC5UZXN0Q2FzZSk6CiAgICAgICAgICMgVGhlIGhhc2ggaGFwcGVucyB0byBiZSB0 aGUgbmFtZSBvZiB0aGUgY2xhc3MsIGJ1dCBzZXRzIHN0aWxsIHdvcms6CiAgICAgICAgIGNyYXNo X3NldCA9IHNldChbRmFpbHVyZUNyYXNoKCksICJGYWlsdXJlQ3Jhc2giXSkKICAgICAgICAgc2Vs Zi5hc3NlcnRFcXVhbChsZW4oY3Jhc2hfc2V0KSwgMikKLQotaWYgX19uYW1lX18gPT0gJ19fbWFp bl9fJzoKLSAgICB1bml0dGVzdC5tYWluKCkKSW5kZXg6IFRvb2xzL1NjcmlwdHMvd2Via2l0cHkv bGF5b3V0X3Rlc3RzL2xheW91dF9wYWNrYWdlL3Rlc3RfcmVzdWx0c191bml0dGVzdC5weQo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9sYXlvdXRfcGFj a2FnZS90ZXN0X3Jlc3VsdHNfdW5pdHRlc3QucHkJKHJldmlzaW9uIDg5ODQ4KQorKysgVG9vbHMv U2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbGF5b3V0X3BhY2thZ2UvdGVzdF9yZXN1bHRz X3VuaXR0ZXN0LnB5CSh3b3JraW5nIGNvcHkpCkBAIC0zMSw3ICszMSw3IEBAIGltcG9ydCB1bml0 dGVzdAogZnJvbSB0ZXN0X3Jlc3VsdHMgaW1wb3J0IFRlc3RSZXN1bHQKIAogCi1jbGFzcyBUZXN0 KHVuaXR0ZXN0LlRlc3RDYXNlKToKK2NsYXNzIFRlc3RSZXN1bHRzVGVzdCh1bml0dGVzdC5UZXN0 Q2FzZSk6CiAgICAgZGVmIHRlc3RfZGVmYXVsdHMoc2VsZik6CiAgICAgICAgIHJlc3VsdCA9IFRl c3RSZXN1bHQoImZvbyIpCiAgICAgICAgIHNlbGYuYXNzZXJ0RXF1YWwocmVzdWx0LmZpbGVuYW1l LCAnZm9vJykKQEAgLTUwLDcgKzUwLDMgQEAgY2xhc3MgVGVzdCh1bml0dGVzdC5UZXN0Q2FzZSk6 CiAKICAgICAgICAgIyBBbHNvIGNoZWNrIHRoYXQgIT0gaXMgaW1wbGVtZW50ZWQuCiAgICAgICAg IHNlbGYuYXNzZXJ0RmFsc2UobmV3X3Jlc3VsdCAhPSByZXN1bHQpCi0KLQotaWYgX19uYW1lX18g PT0gJ19fbWFpbl9fJzoKLSAgICB1bml0dGVzdC5tYWluKCkK