Bug 37782 – new-run-webkit-tests: add a "dryrun" port that can be used to trace expectations on other platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 53643 [details]
add unit tests

[Dirk Pranke]

Created attachment 53644 [details]
fix ChangeLog

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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 ...

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 53730 [details]
update w/ feedback from eseidel

[Eric Seidel (no email)]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 53736 [details]
more feedback from eseidel

[Adam Barth]

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.

[Dirk Pranke]

Created attachment 53742 [details]
feedback from abarth - don't hang read_file() and write_file() off of the port object

[Dirk Pranke]

Created attachment 53743 [details]
whoops. try again w/ correct patch

[Dirk Pranke]

Created attachment 53744 [details]
and yet again ...

[Adam Barth]

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.

[Dirk Pranke]

Created attachment 53750 [details]
final patch using "with" statement; no review needed

[Adam Barth]

 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.

[Dirk Pranke]

Committed r57860: <http://trac.webkit.org/changeset/57860>

[Dirk Pranke]

(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.

[Adam Barth]

Ah, i missed line 59.  That makes more sense.  :)