Bug 52863

Created attachment 79687 [details] Patch

Comment on attachment 79687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79687&action=review Maybe we should think about creating a new file suffix _integrationtest.py or something and having test-webkitpy know how to run them with passed --integration or some-such? This looks good except for the None to [] change. > Tools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:115 > + def get_printer(self, args=[], single_threaded=False, None is preferred for default arguments. The default values are shared by all instances, so if anything ever tried to mutate that args value, we'd have bad news bears. :)

cc'ing some other people in case they have thoughts on this ... 1) regarding integration tests, my thinking had been to modify test-webkitpy to support some sort of @integration decorator that could mark up tests or classes. I would prefer to keep things in a single file because I think that allows for much better reuse of fixtures in between tests. 2) regarding using None instead of [] as a default argument value, Mihai had suggested in another code review a few days ago that [] might be preferable to None in functions where you weren't modifying the argument inside the function (the advantage is that it's clearer and shorter). That said, it's certainly more fragile. Arguably there's a middle ground where it might be safe to use [] if the variable is never passed to another function (so at least it's visible and in your control), but after having thought about it more, I think I'm inclined to agree with you that we should just use None to be safe. Anyone else have any thoughts on either point?

Committed r76398: <http://trac.webkit.org/changeset/76398>

(In reply to comment #3) > 2) regarding using None instead of [] as a default argument value, Mihai had suggested in another code review a few days ago that [] might be preferable to None in functions where you weren't modifying the argument inside the function (the advantage is that it's clearer and shorter). That said, it's certainly more fragile. Arguably there's a middle ground where it might be safe to use [] if the variable is never passed to another function (so at least it's visible and in your control), but after having thought about it more, I think I'm inclined to agree with you that we should just use None to be safe. I don't actually have a strong preference about this, so if you want to always use None (to be safe, in case the function is changed to modify the argument later), I can promise to point it out in reviews :)

(In reply to comment #5) > I can promise to point it out in reviews :) Err, _not_ point it out in reviews.

*** Bug 46322 has been marked as a duplicate of this bug. ***