Bug 52863 - nrwt: suppress remaining integration tests for now
Summary: nrwt: suppress remaining integration tests for now
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
: 46322 (view as bug list)
Depends on:
Blocks: 48072
  Show dependency treegraph
 
Reported: 2011-01-20 20:01 PST by Dirk Pranke
Modified: 2011-02-17 17:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.08 KB, patch)
2011-01-20 20:02 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***