Bug 53126 – nrwt: add a "mock" DRT implementation for better testing of new-run-webkit-tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: add a "mock" DRT implementation for better testing of new-run-webkit-tests

[Dirk Pranke]

Created attachment 80121 [details]
Patch

[Dirk Pranke]

Created attachment 80124 [details]
add bug # to ChangeLog

[Tony Chang]

Comment on attachment 80124 [details]
add bug # to ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=80124&action=review

> Tools/ChangeLog:6
> +        DumpRenderTree is supposed to be, and a --mock-drt flag

Do we have to add a new flag? Can we just use a different platform?  e.g., rather than having to edit chromium.py and webkit.py, can we just add a platform that overrides driver_name (maybe pass in a base port to the ctor)?  I guess the tradeoff is that get() would be more complicated.  I don't know, what do you think?

> Tools/ChangeLog:11
> +        This will eventually replace port/dryrun.py and allow us to get
> +        better test coverage of the new-run-webkit-tests code as well as
> +        a reference for what new-run-webkit-tests expects from DRT.

This sounds like a good goal.

> Tools/Scripts/webkitpy/layout_tests/mock_drt.py:1
> +#!/usr/bin/env python

Is this always going to work on windows/cygwin?  I guess we don't have to handle the no-cygwin case.  Do we have to worry about having the execute bit set?

> Tools/Scripts/webkitpy/layout_tests/mock_drt.py:86
> +                (url, expected_checksum) = self.parse_input(line)

No ()s on the left side of the assignment.

> Tools/Scripts/webkitpy/layout_tests/mock_drt.py:102
> +        if "'" in line:
> +            return line.split("'")

line.split("'", 1)?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 80124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80124&action=review
> 
> > Tools/ChangeLog:6
> > +        DumpRenderTree is supposed to be, and a --mock-drt flag
> 
> Do we have to add a new flag? Can we just use a different platform?  e.g., rather than having to edit chromium.py and webkit.py, can we just add a platform that overrides driver_name (maybe pass in a base port to the ctor)?  I guess the tradeoff is that get() would be more complicated.  I don't know, what do you think?

It's a cross-cutting flag, so it'll affect pretty much every port. I think I can probably implement this with a delegate (much like DryRunPort does today). I'll look into it.

Ideally I'd check this in as is and look at replacing it with a different platform later. What do you think?

> > Tools/Scripts/webkitpy/layout_tests/mock_drt.py:1
> > +#!/usr/bin/env python
> 
> Is this always going to work on windows/cygwin?  I guess we don't have to handle the no-cygwin case.  Do we have to worry about having the execute bit set?
>

On Windows I'll probably launch python directly and pass it the argument. We will probably want to make the non-cygwin case work since this'll eventually be enabled as part of the unit/integration tests.
 
> > Tools/Scripts/webkitpy/layout_tests/mock_drt.py:86
> > +                (url, expected_checksum) = self.parse_input(line)
> 
> No ()s on the left side of the assignment.
>

Will do.
 
> > Tools/Scripts/webkitpy/layout_tests/mock_drt.py:102
> > +        if "'" in line:
> > +            return line.split("'")
> 
> line.split("'", 1)?

Will do.

[Dirk Pranke]

Created attachment 80155 [details]
replace --mock-drt flag with mock-* port implementation

[Tony Chang]

Comment on attachment 80155 [details]
replace --mock-drt flag with mock-* port implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=80155&action=review

r=me with the following changes.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:41
> +import base
> +import factory

This should include the full import path from Scripts.  E.g., see how files in webkitpy/common/system import other files from webkitpy/common/system.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:50
> +        pfx = 'mock-'

pfx -> prefix

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:138
> +class MockDRT(object):
> +    def __init__(self, options, args, stdin, stdout, stderr):

Please add a docstring.  At some point, we may want to move this into a separate file. Maybe we're not there yet.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:38
> +import mock_drt
> +import factory
> +import port_testcase

These should also be imports with the full path.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:87
> +            drt_output = """Content-Type: text/plain
> +%s#EOF
> +
> +ActualHash: %s
> +ExpectedHash: %s
> +#EOF
> +""" % (text_output, expected_checksum, expected_checksum)

I find code like this easier to read when the indention is preserved.  E.g.,
    drt_output = (
        "Content-Type: text/plain\n"
        "%s#EOF\n"
        ... ") % (text_output, expected_checksum, expected_checksum)

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:102
> +        res = drt.run()

res -> result

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 80155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80155&action=review
> 
> r=me with the following changes.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:41
> > +import base
> > +import factory
> 
> This should include the full import path from Scripts.  E.g., see how files in webkitpy/common/system import other files from webkitpy/common/system.
> 

I can do this, but why do you think that this is better?

> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:50
> > +        pfx = 'mock-'
> 
> pfx -> prefix
>

Ok.
 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:138
> > +class MockDRT(object):
> > +    def __init__(self, options, args, stdin, stdout, stderr):
> 
> Please add a docstring.  At some point, we may want to move this into a separate file. Maybe we're not there yet.
>

Ok. I actually had these in two files initially, but thought it might be clearer or better to have them in a single file. Do you think two is better?
 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:38
> > +import mock_drt
> > +import factory
> > +import port_testcase
> 
> These should also be imports with the full path.
> 

As above.

> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:87
> > +            drt_output = """Content-Type: text/plain
> > +%s#EOF
> > +
> > +ActualHash: %s
> > +ExpectedHash: %s
> > +#EOF
> > +""" % (text_output, expected_checksum, expected_checksum)
> 
> I find code like this easier to read when the indention is preserved.  E.g.,
>     drt_output = (
>         "Content-Type: text/plain\n"
>         "%s#EOF\n"
>         ... ") % (text_output, expected_checksum, expected_checksum)
>

Okay.
 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:102
> > +        res = drt.run()
> 
> res -> result

Okay.

[Tony Chang]

Explicit imports are better because it's easier to find the file being imported.  It also makes it clear what is a system library and what is a library we wrote.  It also avoids problems with files shadowing other files.

I think one file is fine for now.  We can revisit as the class grows.

[Dirk Pranke]

(In reply to comment #8)
> Explicit imports are better because it's easier to find the file being imported.  It also makes it clear what is a system library and what is a library we wrote.  It also avoids problems with files shadowing other files.

These are good points. On the other hand, this file is intended to be (using java terminology) package-private, and it references other package-private files (factory, base), and specifying the full paths would probably lead people to think otherwise. In addition, because there are symbols exported from port/__init__.py, I think importing things through the full path can cause symbol and namespace conflicts. Does that make sense?

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > Explicit imports are better because it's easier to find the file being imported.  It also makes it clear what is a system library and what is a library we wrote.  It also avoids problems with files shadowing other files.
> 
> These are good points. On the other hand, this file is intended to be (using java terminology) package-private, and it references other package-private files (factory, base), and specifying the full paths would probably lead people to think otherwise. In addition, because there are symbols exported from port/__init__.py, I think importing things through the full path can cause symbol and namespace conflicts. Does that make sense?

(Note that I will make the change as you suggested, I'm just trying to understand the tradeoffs better).

[Dirk Pranke]

Hm. PEP 8 requires full or absolute imports, and there is a discussion of this in PEP 328:

http://www.python.org/dev/peps/pep-0328/

In fact, it looks like relative imports might actually break in Python 3. It looks like "__all__" should be used to address the public/private thing I am concerned about.

It looks like "from . import factory" might also be acceptable. What do you think of this alternative?

[Dirk Pranke]

Created attachment 80224 [details]
update w/ review feedback from tony

[Dirk Pranke]

Committed r76709: <http://trac.webkit.org/changeset/76709>

[Tony Chang]

(In reply to comment #11)
> Hm. PEP 8 requires full or absolute imports, and there is a discussion of this in PEP 328:
> 
> http://www.python.org/dev/peps/pep-0328/
> 
> In fact, it looks like relative imports might actually break in Python 3. It looks like "__all__" should be used to address the public/private thing I am concerned about.
> 
> It looks like "from . import factory" might also be acceptable. What do you think of this alternative?

I think the full path is more consistent with the rest of our code.

I am not convinced that an import usage in X.py will be sufficient to stop people from importing a 'private' Y.py.  Actually, I'm not sure we should bother to even have such a distinction of private packages.

[Dirk Pranke]

(In reply to comment #14)
> I am not convinced that an import usage in X.py will be sufficient to stop people from importing a 'private' Y.py.  Actually, I'm not sure we should bother to even have such a distinction of private packages.

I think that the port/* package is a pretty good example of something where you really only want people calling one or two routines and not whatever they happen to find lying around. some concept of public/private is a good way to document the intended usage and/or have check-webkit-style or other linters complain.

I agree that there's not many examples of this, though. I'm not even sure if there are any others.

[Csaba Osztrogonác]

r76709 made one webkitpy test fail on Qt and GTK bots:

ERROR: test_pixeltest (webkitpy.layout_tests.port.mock_drt_unittest.MockDRTTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 108, in test_pixeltest
    self.assertTest('fast/html/keygen.html', True)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py", line 104, in assertTest
    self.assertEqual(stdout.getvalue(), drt_output)
  File "/usr/lib/python2.5/StringIO.py", line 270, in getvalue
    self.buf += ''.join(self.buflist)
UnicodeDecodeError: 'ascii' codec can't decode byte 0x89 in position 0: ordinal not in range(128)

[Dirk Pranke]

Created attachment 80278 [details]
Patch

[Tony Chang]

(In reply to comment #17)
> Created an attachment (id=80278) [details]
> Patch

There seem to be a lot of changes from the previous patch.  Can you summarize the changes?  Alternately, you could have the new patch have the minimal number of changes to work and add a follow up change with more refactoring/adding test cases.  I like the extra test cases, but it's not clear to me which were related to the test breakage.

[Dirk Pranke]

Created attachment 80354 [details]
delta from r76709

[Dirk Pranke]

Okay, I've uploaded the delta from the patch that was committed, for easier review. There were two problems with the patch that got committed. The first was that DRT (and MockDRT) writes 8-bit strings to stdout (including the PNG output), but the MockDRT port was attempting to combine all of the output using StringIO. Some of the output in MockDRT is actually unicode, so it was attempting to convert the 8-bit to unicode while concatenating all of the text together (in stdout.getvalue()), and choking. The fix is to not concatenate the files and compare the individual write()s (in the .buflist field).

Alternatively I could try to get MockDRT to only output raw strings and hope that StringIO doesn't decide that some of them should be unicode and try to convert them. I'm looking for feedback here.

Now, this problem was triggered by the fact that I was using the actual snow-leopard expectations, but had a bug where instead of reading the snow-leopard "actuals", I was using whatever the default port was (linux, mac-leopard, etc). So, I fixed that bug (in MockDRTPortTest.make_drt()), but also switched from the mac-snowleopard expectations to the test port expectations, to be platform- and filesystem- independent.

Fixing these things made me realize that I didn't have a test case for when the checksums mismatched and DRT actually output the PNG, so I added that, and I modified the TestPort implementation to include non-unicode characters in the PNG output.

I then ran some coverage tests and added tests for the other stuff that was missing (mostly the stub routines like start_http_server on both mock_drt.py and dryrun.py. I also changed the syntax so that --platform mock won't work, you have to say --platform mock-X to be explicit about which port you want to use.

[Tony Chang]

Comment on attachment 80278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80278&action=review

I'm still a bit confused as to where we need to use unicode().  From your description, it sounds like we don't need to use unicode() since you use .buflist?

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:189
> +            self._stdout.write("ExpectedHash: %s\n" % unicode(expected_checksum))

Do we need the call to unicode() here if we're using .buflist?  If so, why doesn't |actual_checksum| need to call it?

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:46
> +    def test_port_name_in_constructor(self):
> +        self.assertTrue(mock_drt.MockDRTPort(port_name='mock-test'))

This will always return an object unless there's an exception.  Maybe just remove the assertTrue call?

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:146
> +    def assertTest(self, test_name, pixel_tests, expected_checksum=None, drt_output=None):

Nit: Maybe name this test something other than assertTest.  It kind of looks like one of the assertion methods in unittest.TestCase.

> Tools/Scripts/webkitpy/layout_tests/port/test.py:55
> +        self.actual_text = unicode(self.base + '-txt')
> +        self.actual_checksum = unicode(self.base + '-checksum')

I don't understand why we need the unicode() calls here.  Is this change related to the mock drt failures?  If not, why is it in this change?

[Dirk Pranke]

(In reply to comment #21)
> (From update of attachment 80278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80278&action=review
> 
> I'm still a bit confused as to where we need to use unicode().  From your description, it sounds like we don't need to use unicode() since you use .buflist?
> 

Fair enough. I can get rid of all of the unicode() calls and u"" literals and the tests will still pass. It does however obscure the issue around StringIO's handling of a mixture of the two types. There doesn't appear to be a way to tell StringIO to *only* use 8-bit (non-encoded) strings. I thought that perhaps explicitly mixing unicode and non-unicode might be a little clearer about this, but maybe this is adequately addressed through just having the comment near the .buflist.

> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:189
> > +            self._stdout.write("ExpectedHash: %s\n" % unicode(expected_checksum))
> 
> Do we need the call to unicode() here if we're using .buflist?  If so, why doesn't |actual_checksum| need to call it?
>

See above. actual_checksum, coming from the split routine called previously, is already unicode.
 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:46
> > +    def test_port_name_in_constructor(self):
> > +        self.assertTrue(mock_drt.MockDRTPort(port_name='mock-test'))
> 
> This will always return an object unless there's an exception.  Maybe just remove the assertTrue call?
> 

Good point. Will do.

> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:146
> > +    def assertTest(self, test_name, pixel_tests, expected_checksum=None, drt_output=None):
> 
> Nit: Maybe name this test something other than assertTest.  It kind of looks like one of the assertion methods in unittest.TestCase.
>

It was supposed to :) Would it be better to call it something like assertOneTestRunsFine(), or would you prefer it not to be called assert* at all?

> > Tools/Scripts/webkitpy/layout_tests/port/test.py:55
> > +        self.actual_text = unicode(self.base + '-txt')
> > +        self.actual_checksum = unicode(self.base + '-checksum')
> 
> I don't understand why we need the unicode() calls here.  Is this change related to the mock drt failures?  If not, why is it in this change?

Yes, it is related. I encode these as unicode to ensure that it replicates the bug we saw with mixing unicode and non-unicode strings. Would adding a comment here help? I thought I had added a comment about the \x89 in the png string, but I didn't, and it seems like that at the very least should be there.

[Tony Chang]

(In reply to comment #22)
> I thought that perhaps explicitly mixing unicode and non-unicode might be a little clearer about this, but maybe this is adequately addressed through just having the comment near the .buflist.

I think the mixed usage is confusing me.  If I understand correctly, the problem is that we are putting unicode() strings and unencoded strings into a StringIO buffer and that is confusing StringIO.  Can we only put unencoded strings into the StringIO buffer since the image data is unencoded?  E.g., on unicode strings, call .encode('ascii') or .encode('utf8')?

> It was supposed to :) Would it be better to call it something like assertOneTestRunsFine(), or would you prefer it not to be called assert* at all?

Oh, I see.  Yeah, this is fine.  Maybe something like assertOneTestRun?

> > > Tools/Scripts/webkitpy/layout_tests/port/test.py:55
> > > +        self.actual_text = unicode(self.base + '-txt')
> > > +        self.actual_checksum = unicode(self.base + '-checksum')
> > 
> > I don't understand why we need the unicode() calls here.  Is this change related to the mock drt failures?  If not, why is it in this change?
> 
> Yes, it is related. I encode these as unicode to ensure that it replicates the bug we saw with mixing unicode and non-unicode strings. Would adding a comment here help? I thought I had added a comment about the \x89 in the png string, but I didn't, and it seems like that at the very least should be there.

Seems like it would be better to not mix unicode and non-unicode.

[Dirk Pranke]

(In reply to comment #23)
> (In reply to comment #22)
> > I thought that perhaps explicitly mixing unicode and non-unicode might be a little clearer about this, but maybe this is adequately addressed through just having the comment near the .buflist.
> 
> I think the mixed usage is confusing me.  If I understand correctly, the problem is that we are putting unicode() strings and unencoded strings into a StringIO buffer and that is confusing StringIO.  Can we only put unencoded strings into the StringIO buffer since the image data is unencoded?  E.g., on unicode strings, call .encode('ascii') or .encode('utf8')?

DRT mixes MIME types on its output (text/plain and image/png). I don't know if it is defined that DRT is only supposed to return ASCII, or UTF-8, or if it could be either. But, NRWT reads the text output as if it is UTF-8, so that appears to work.

So, DRT already mixes unicode and non-unicode. I think the unit tests should reflect this and do the same thing.

> > It was supposed to :) Would it be better to call it something like assertOneTestRunsFine(), or would you prefer it not to be called assert* at all?
> 
> Oh, I see.  Yeah, this is fine.  Maybe something like assertOneTestRun?
>

Sure.
 
> > > > Tools/Scripts/webkitpy/layout_tests/port/test.py:55
> > > > +        self.actual_text = unicode(self.base + '-txt')
> > > > +        self.actual_checksum = unicode(self.base + '-checksum')
> > > 
> > > I don't understand why we need the unicode() calls here.  Is this change related to the mock drt failures?  If not, why is it in this change?
> > 
> > Yes, it is related. I encode these as unicode to ensure that it replicates the bug we saw with mixing unicode and non-unicode strings. Would adding a comment here help? I thought I had added a comment about the \x89 in the png string, but I didn't, and it seems like that at the very least should be there.
> 
> Seems like it would be better to not mix unicode and non-unicode.

As per above.

[Dirk Pranke]

Created attachment 80387 [details]
put the encoding/decoding of utf-8 where it should be, in filesystem_mock and MockDRT()

[Dirk Pranke]

Okey, in a conversation over IM tony and eventually worked out that we think the most realistic way to reproduce what the real code is doing is to have test.py store values as raw byte strings (no unicode casts or literals), rely on filesystem_mock to do the conversion to unicode, and to have the MockDRT code explicitly convert the unicode strings *back* into byte strings to be written to "stdout".

As a result, it probably should be safe to use StringIO.getvalue() since everything should be raw strings, but I'll leave the code as a buflist for now.

You can mostly disregard the diffs in test.py, they're just indentation cleanup. However, look at the comments in TestInstance() over how we expect unicode to be handled. And, this actually revealed a bug in the mock diff_image implementation, which was writing a text file instead of a binary file.

[Tony Chang]

Comment on attachment 80387 [details]
put the encoding/decoding of utf-8 where it should be, in filesystem_mock and MockDRT()

View in context: https://bugs.webkit.org/attachment.cgi?id=80387&action=review

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:180
> +        actual_text = port.expected_text(test_path).encode('utf-8')
> +        if self._options.pixel_tests and expected_checksum:
> +            actual_checksum = port.expected_checksum(test_path).encode('utf-8')
> +            actual_image = port.expected_image(test_path)

Nit: Rather than calling encode('utf-8') in lots of places, maybe there should be a helper "write_text_file_data_to_stdout" that does the conversion.

[Dirk Pranke]

Committed r76982: <http://trac.webkit.org/changeset/76982>

[Dirk Pranke]

(In reply to comment #27)
> (From update of attachment 80387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80387&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:180
> > +        actual_text = port.expected_text(test_path).encode('utf-8')
> > +        if self._options.pixel_tests and expected_checksum:
> > +            actual_checksum = port.expected_checksum(test_path).encode('utf-8')
> > +            actual_image = port.expected_image(test_path)
> 
> Nit: Rather than calling encode('utf-8') in lots of places, maybe there should be a helper "write_text_file_data_to_stdout" that does the conversion.

Good suggestion. I decided to do something slightly different and added a raw_bytes() method and changed a bunch of variable names to make it clearer that we were writing byte strings rather than unicode strings.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76982 might have broken Leopard Intel Release (Tests)

[Jessie Berlin]

I think these changes have broken the Windows 7 Release Tests

http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8769
http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8770

[Dirk Pranke]

(In reply to comment #31)
> I think these changes have broken the Windows 7 Release Tests
> 
> http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8769
> http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8770

I checked in a temporary fix to work around this in r77023. I will upload a different patch with a cleaner fix for review, but hopefully this unblocks things and keeps us from having to revert this patch.

[Dirk Pranke]

Committed r77038: <http://trac.webkit.org/changeset/77038>