Bug 37782 - new-run-webkit-tests: add a "dryrun" port that can be used to trace expectations on other platforms
Summary: new-run-webkit-tests: add a "dryrun" port that can be used to trace expectati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-18 16:26 PDT by Dirk Pranke
Modified: 2010-04-19 18:02 PDT (History)
5 users (show)

See Also:


Attachments
add unit tests (16.35 KB, patch)
2010-04-18 16:40 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix ChangeLog (15.89 KB, patch)
2010-04-18 16:42 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ feedback from eseidel (17.31 KB, patch)
2010-04-19 15:41 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
more feedback from eseidel (17.41 KB, patch)
2010-04-19 16:32 PDT, Dirk Pranke
abarth: review-
Details | Formatted Diff | Diff
feedback from abarth - don't hang read_file() and write_file() off of the port object (2.94 KB, patch)
2010-04-19 17:03 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
whoops. try again w/ correct patch (17.99 KB, patch)
2010-04-19 17:06 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
and yet again ... (deleted)
2010-04-19 17:10 PDT, Dirk Pranke
abarth: review+
Details | Formatted Diff | Diff
final patch using "with" statement; no review needed (17.13 KB, patch)
2010-04-19 17:48 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-04-18 16:26:38 PDT
We should repurpose the Passing port to be able to stub out and run ports other than the one we're currently running on. Currently the Passing implementation still checks for a build and starts and stops the auxiliary services. If we stub that all out, then we can do a full dry run of any port, which can be useful for checking baselines, coverage testing, etc.
Comment 1 Dirk Pranke 2010-04-18 16:40:59 PDT
Created attachment 53643 [details]
add unit tests
Comment 2 Dirk Pranke 2010-04-18 16:42:53 PDT
Created attachment 53644 [details]
fix ChangeLog
Comment 3 Eric Seidel (no email) 2010-04-18 16:45:53 PDT
Comment on attachment 53644 [details]
fix ChangeLog

What encoding is the text_expectations.txt file in?  I would assume UTF8.  We'll need to be explicit in our open() calls, probably easiest to use codecs.open(name, mode, encoding) instead of having to convert each read into a unicode string.
Comment 4 Dirk Pranke 2010-04-18 16:53:19 PDT
(In reply to comment #3)
> (From update of attachment 53644 [details])
> What encoding is the text_expectations.txt file in?  I would assume UTF8. 
> We'll need to be explicit in our open() calls, probably easiest to use
> codecs.open(name, mode, encoding) instead of having to convert each read into a
> unicode string.

I think we generally assume test_expectations is in ASCII, like most source code. I don't think there's a strong reason to allow non-ASCII characters in it, in which case the default open() calls should work just fine.

I'm also not sure what that comment has to do with this particular change ...
Comment 5 Eric Seidel (no email) 2010-04-19 00:38:47 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 53644 [details] [details])
> > What encoding is the text_expectations.txt file in?  I would assume UTF8. 
> > We'll need to be explicit in our open() calls, probably easiest to use
> > codecs.open(name, mode, encoding) instead of having to convert each read into a
> > unicode string.
> 
> I think we generally assume test_expectations is in ASCII, like most source
> code. I don't think there's a strong reason to allow non-ASCII characters in
> it, in which case the default open() calls should work just fine.
> 
> I'm also not sure what that comment has to do with this particular change ...

It was made simply because unicode() is on my mind at the moment after having written bug 37765.  The default open() calls will return a file object for which read() returns a str() (which is a byte-stream and not a really a "string").  If test_expectations are all ascii then that's fine, but technically we should be chosing an encoding and converting to unicode either during or after read().

Either way, as you said, it doesn't necessarily pertain to the guts of this change.
Comment 6 Eric Seidel (no email) 2010-04-19 00:44:46 PDT
Comment on attachment 53644 [details]
fix ChangeLog

I don't think "dryrun" is a word.  In which case, the class would be DryRun no?

This leaks a file handle, no?
+            text_output = open(text_filename, 'r').read()

And here?
+                image = file(image_filename, 'rb').read()

and several more...

I think in python 2.6 that might auto-cleanup for you, but I don't think earlier versions of python do.

You could use "with" to have 2.5+ auto-close the file for you.

I'm not sure I understand what:
+    def _uri_to_test(self, uri):

is trying to do.

Does this not start up actual websocket/apache servers?  I would think those would be important for using this for profiling.
Comment 7 Dirk Pranke 2010-04-19 15:26:57 PDT
(In reply to comment #6)
> (From update of attachment 53644 [details])
> I don't think "dryrun" is a word.  In which case, the class would be DryRun no?
> 

Perhaps. I'll change it.

> This leaks a file handle, no?
> +            text_output = open(text_filename, 'r').read()
> 
> And here?
> +                image = file(image_filename, 'rb').read()
> 
> and several more...
>

I will fix those.
 
> I'm not sure I understand what:
> +    def _uri_to_test(self, uri):
>
> is trying to do.
>

It remaps full URIs to the actual underlying test names, so that we can then look up the expected baselines for the test. I will add better comments.
 
> Does this not start up actual websocket/apache servers?  I would think those
> would be important for using this for profiling.

No, it doesn't. That is one of the main differences between this implementation and the previous "passing" implementation. I'm not sure how useful this is. I suppose if you wanted to time a test run that assumed an infinitely fast dumprendertree but regular execution of everything else, you might want that.
Comment 8 Dirk Pranke 2010-04-19 15:41:29 PDT
Created attachment 53730 [details]
update w/ feedback from eseidel
Comment 9 Eric Seidel (no email) 2010-04-19 16:10:15 PDT
Comment on attachment 53730 [details]
update w/ feedback from eseidel

read_file on port seems like the wrong abstraction.

You're clearly repeating the same pattern:
+        text_filename = self._port.expected_filename(test_name, '.txt')
+        try:
+            text_output = self._port.read_file(text_filename)
+        except IOError:
+            text_output = ''

Why not just have a helper function which does that?  Just a free function in the same file.

Also, why not use the 2.5+ "with" sequence to make the explicit file.close() uneeded?
Comment 10 Dirk Pranke 2010-04-19 16:25:32 PDT
(In reply to comment #9)
> (From update of attachment 53730 [details])
> read_file on port seems like the wrong abstraction.

I originally had it in the same file, but then I realized that other ports might want to be able to do the same thing, and the Port class is a nice place to hang platform-specific stuff of of.

> 
> You're clearly repeating the same pattern:
> +        text_filename = self._port.expected_filename(test_name, '.txt')
> +        try:
> +            text_output = self._port.read_file(text_filename)
> +        except IOError:
> +            text_output = ''
> 
> Why not just have a helper function which does that?  Just a free function in
> the same file.
>

Makes sense. I'll add that.
 
> Also, why not use the 2.5+ "with" sequence to make the explicit file.close()
> uneeded?

I haven't gotten used to using with yet. It feels wrong or clunky to me, somehow.
Comment 11 Dirk Pranke 2010-04-19 16:26:37 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 53730 [details] [details])
> > read_file on port seems like the wrong abstraction.
> 
> I originally had it in the same file, but then I realized that other ports
> might want to be able to do the same thing, and the Port class is a nice place
> to hang platform-specific stuff of of.

Also, you're right that it could be a free function instead of a method on the Port class, but we don't have a place for platform-specific functions lying around AFAIK.
Comment 12 Dirk Pranke 2010-04-19 16:32:11 PDT
Created attachment 53736 [details]
more feedback from eseidel
Comment 13 Adam Barth 2010-04-19 16:36:24 PDT
Comment on attachment 53736 [details]
more feedback from eseidel

+ def read_file(self, path, mode='r'):

I don't read_file and write_file should be on the Port object.  They don't have anything to do with WebKit ports.

In general, we shouldn't try to make Port the all-sing, all-dance wrapper for the world.
Comment 14 Dirk Pranke 2010-04-19 17:03:01 PDT
Created attachment 53742 [details]
feedback from abarth - don't hang read_file() and write_file() off of the port object
Comment 15 Dirk Pranke 2010-04-19 17:06:18 PDT
Created attachment 53743 [details]
whoops. try again w/ correct patch
Comment 16 Dirk Pranke 2010-04-19 17:10:32 PDT
Created attachment 53744 [details]
and yet again ...
Comment 17 Adam Barth 2010-04-19 17:15:16 PDT
Comment on attachment 53744 [details]
and yet again ...

Nifty!

 58         f = open(path, mode)
 59         contents = f.read()
 60         f.close()

This might leak a file handle when an exception is thrown.  Please use the "with" construction.
Comment 18 Dirk Pranke 2010-04-19 17:48:38 PDT
Created attachment 53750 [details]
final patch using "with" statement; no review needed
Comment 19 Adam Barth 2010-04-19 17:53:20 PDT
 62             contents = f.read()

I'm surprised you don't need to declare contents in the top-most scope of this function, but I presume you ran this fine.  I guess I still don't quite understand Python scoping rules.
Comment 20 Dirk Pranke 2010-04-19 17:53:31 PDT
Committed r57860: <http://trac.webkit.org/changeset/57860>
Comment 21 Dirk Pranke 2010-04-19 18:00:04 PDT
(In reply to comment #19)
>  62             contents = f.read()
> 
> I'm surprised you don't need to declare contents in the top-most scope of this
> function, but I presume you ran this fine.  I guess I still don't quite
> understand Python scoping rules.

contents was declared and initialized on line 59. I think it would've worked in the regular (non-exceptional case) because the variable would've still been live (I.e., neither the try nor the with introduce true block scopes like def does). But, it certainly would've failed in the exception case.
Comment 22 Adam Barth 2010-04-19 18:02:24 PDT
Ah, i missed line 59.  That makes more sense.  :)