Summary: | new-run-webkit-tests: add a "dryrun" port that can be used to trace expectations on other platforms | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, cjerdonek, eric, ojan, tony | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Dirk Pranke
2010-04-18 16:26:38 PDT
Created attachment 53643 [details]
add unit tests
Created attachment 53644 [details]
fix ChangeLog
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.
(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 ... (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 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.
(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. Created attachment 53730 [details]
update w/ feedback from eseidel
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?
(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. (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. Created attachment 53736 [details]
more feedback from eseidel
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.
Created attachment 53742 [details]
feedback from abarth - don't hang read_file() and write_file() off of the port object
Created attachment 53743 [details]
whoops. try again w/ correct patch
Created attachment 53744 [details]
and yet again ...
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.
Created attachment 53750 [details]
final patch using "with" statement; no review needed
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. Committed r57860: <http://trac.webkit.org/changeset/57860> (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. Ah, i missed line 59. That makes more sense. :) |