Bug 52863

Summary: nrwt: suppress remaining integration tests for now
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, mihai, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 48072    
Attachments:
Description Flags
Patch eric: review+

Description Dirk Pranke 2011-01-20 20:01:33 PST
nrwt: suppress remaining integration tests for now
Comment 1 Dirk Pranke 2011-01-20 20:02:02 PST
Created attachment 79687 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-01-21 02:40:44 PST
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. :)
Comment 3 Dirk Pranke 2011-01-21 10:18:49 PST
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?
Comment 4 Dirk Pranke 2011-01-21 15:23:59 PST
Committed r76398: <http://trac.webkit.org/changeset/76398>
Comment 5 Mihai Parparita 2011-01-21 15:45:49 PST
(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 :)
Comment 6 Mihai Parparita 2011-01-21 15:49:07 PST
(In reply to comment #5)
> I can promise to point it out in reviews :)

Err, _not_ point it out in reviews.
Comment 7 Dirk Pranke 2011-02-17 17:33:05 PST
*** Bug 46322 has been marked as a duplicate of this bug. ***