Bug 47879 – FAIL: test_reset_results (webkitpy.layout_tests.run_webkit_tests_unittest.RebaselineTest)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

======================================================================
FAIL: test_reset_results (webkitpy.layout_tests.run_webkit_tests_unittest.RebaselineTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 298, in test_reset_results
    "data/passes/image")
  File "/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 281, in assertBaselines
    self.assertTrue(any(f.find(baseline) != -1 for f in file_list))
AssertionError

----------------------------------------------------------------------

I'm disabling the test for now.

[Dirk Pranke]

Created attachment 71134 [details]
Patch

[Adam Barth]

Comment on attachment 71134 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:171
> +        return base.Port.maybe_make_directory(self, *path)

Call-through mocks are dangerous because they can have unexpected side-effects.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:71
> +# FIXME: This makes StringIO objects work with "with". Remove
> +# when we upgrade to 2.6.
> +class NewStringIO(StringIO.StringIO):
> +    def __enter__(self):
> +        return self
> +
> +    def __exit__(self, type, value, traceback):
> +        pass

This seems like a very general concept.  Is there a reason it's in this very specific file?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:306
> +                         '--child-processes=1',

I don't understand why this is related.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 71134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71134&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:171
> > +        return base.Port.maybe_make_directory(self, *path)
> 
> Call-through mocks are dangerous because they can have unexpected side-effects.
>

Agreed. Hence the FIXME. If we don't have the fallthrough, all sorts of weird things will break in the meantime.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:71
> > +# FIXME: This makes StringIO objects work with "with". Remove
> > +# when we upgrade to 2.6.
> > +class NewStringIO(StringIO.StringIO):
> > +    def __enter__(self):
> > +        return self
> > +
> > +    def __exit__(self, type, value, traceback):
> > +        pass
> 
> This seems like a very general concept.  Is there a reason it's in this very specific file?
> 

No, in fact this was cloned from port/base_unittest.py. I would be happy to put it in webkitpy/common somewhere if you thought that was a better place for it.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:306
> > +                         '--child-processes=1',
> 
> I don't understand why this is related.

It's not. I accidentally left that in from debugging the issue.

[Dirk Pranke]

Created attachment 71136 [details]
Patch

[Adam Barth]

Comment on attachment 71136 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:71
> +# FIXME: This makes StringIO objects work with "with". Remove
> +# when we upgrade to 2.6.
> +class NewStringIO(StringIO.StringIO):
> +    def __enter__(self):
> +        return self
> +
> +    def __exit__(self, type, value, traceback):
> +        pass

If this is copy/pasted from somewhere else, that's pretty strong evidence we should move it into somewhere more general.  Maybe webkitpy.common.system ?

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 71136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71136&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:71
> > +# FIXME: This makes StringIO objects work with "with". Remove
> > +# when we upgrade to 2.6.
> > +class NewStringIO(StringIO.StringIO):
> > +    def __enter__(self):
> > +        return self
> > +
> > +    def __exit__(self, type, value, traceback):
> > +        pass
> 
> If this is copy/pasted from somewhere else, that's pretty strong evidence we should move it into somewhere more general.  Maybe webkitpy.common.system ?

There's nothing system-y about it. How about just webkitpy.common.newstringio ?

[Dirk Pranke]

Created attachment 71137 [details]
Patch

[Dirk Pranke]

Created attachment 71138 [details]
Patch

[Kenneth Russell]

(In reply to comment #0)
> ======================================================================
> FAIL: test_reset_results (webkitpy.layout_tests.run_webkit_tests_unittest.RebaselineTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 298, in test_reset_results
>     "data/passes/image")
>   File "/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 281, in assertBaselines
>     self.assertTrue(any(f.find(baseline) != -1 for f in file_list))
> AssertionError
> 
> ----------------------------------------------------------------------
> 
> I'm disabling the test for now.

Did my change in http://trac.webkit.org/changeset/70021 provoke this failure? It didn't occur on my machine.

[Adam Barth]

> Did my change in http://trac.webkit.org/changeset/70021 provoke this failure? It didn't occur on my machine.

Yes.  It didn't occur on my machine either.  The problem is that this test touch the real file system so it has dependencies on the state of your checkout.  That means it's likely to pass locally but fail elsewhere.

[Kenneth Russell]

(In reply to comment #10)
> > Did my change in http://trac.webkit.org/changeset/70021 provoke this failure? It didn't occur on my machine.
> 
> Yes.  It didn't occur on my machine either.  The problem is that this test touch the real file system so it has dependencies on the state of your checkout.  That means it's likely to pass locally but fail elsewhere.

Sorry about the breakage. Adam, do you want to complete the review? Dirk's changes look like an improvement to me even though there's a remaining FIXME about fully virtualizing the file system for these unit tests.

[Adam Barth]

> Sorry about the breakage.

No problem.  We all break things.

> Adam, do you want to complete the review? Dirk's changes look like an improvement to me even though there's a remaining FIXME about fully virtualizing the file system for these unit tests.

I'm not that excited about the patch because it doesn't fix the underlying problem, but someone else should feel free to review it.

[Dirk Pranke]

(In reply to comment #12)
> > Sorry about the breakage.
> 
> No problem.  We all break things.
> 
> > Adam, do you want to complete the review? Dirk's changes look like an improvement to me even though there's a remaining FIXME about fully virtualizing the file system for these unit tests.
> 
> I'm not that excited about the patch because it doesn't fix the underlying problem, but someone else should feel free to review it.

Well, I wouldn't go that far. (a) it's not 100% clear what caused this test to fail, since none of us have been able to reproduce it locally. (b) the test was definitely broken in one way, in that stuff was getting written out when it shouldn't have been. That is now fixed.

It is true that there are still places where things get written out, but they're not places that are going to cause this test to fail, so I'm as sure as I can be that it's fixed without being able to reproduce the exact failure.

[Kenneth Russell]

Comment on attachment 71138 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:275
>          return original_open(name, mode, encoding)

If we are just tossing the contents of the baselines into this in-memory buffer, then for the case of reading them, does it make sense to ever call original_open? Or should we do something similar to the writing case, perhaps adding an entry to a map from name to StringIO object?

[Dirk Pranke]

(In reply to comment #14)
> (From update of attachment 71138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71138&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:275
> >          return original_open(name, mode, encoding)
> 
> If we are just tossing the contents of the baselines into this in-memory buffer, then for the case of reading them, does it make sense to ever call original_open? Or should we do something similar to the writing case, perhaps adding an entry to a map from name to StringIO object?

The problem is that by overwriting codecs.open, this affects anything that attempts to write anywhere, and there are parts of the code that write to the layout-test-results directory, and I'm not 100% sure what will happen if those get mocked as well, since we aren't symmetrically also trying to read from the stringio buffers.

The other patch I have (posted to bug 46776) is the full solution that does all of the above, but it's much more involved so I don't want to try and splice it in here. If no one is happy with the fix above, it won't kill us to just wait for the proper variant of 46776 to land to fix the "root cause" :)

[Kenneth Russell]

Comment on attachment 71138 [details]
Patch

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

>>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:275
>>>          return original_open(name, mode, encoding)
>> 
>> If we are just tossing the contents of the baselines into this in-memory buffer, then for the case of reading them, does it make sense to ever call original_open? Or should we do something similar to the writing case, perhaps adding an entry to a map from name to StringIO object?
> 
> The problem is that by overwriting codecs.open, this affects anything that attempts to write anywhere, and there are parts of the code that write to the layout-test-results directory, and I'm not 100% sure what will happen if those get mocked as well, since we aren't symmetrically also trying to read from the stringio buffers.
> 
> The other patch I have (posted to bug 46776) is the full solution that does all of the above, but it's much more involved so I don't want to try and splice it in here. If no one is happy with the fix above, it won't kill us to just wait for the proper variant of 46776 to land to fix the "root cause" :)

Given this I think the right thing to do is to land all of the pieces associated with bug 46776. I have a feeling that these partial fixes won't really fix the underlying test failure.

[Dirk Pranke]

Comment on attachment 71138 [details]
Patch

will resubmit a new patch once the other refactorings have landed.

[Dirk Pranke]

Closing. This was fixed as of r76321 (bug 52762), if not slightly earlier. These tests no longer have any side effects on the environment / file system / etc.