Bug 44559 – new-run-webkit-tests: clean up code for test_types, test_failures

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: clean up code for test_types, test_failures

[Dirk Pranke]

Created attachment 65334 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 65334 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:76
 +          raise NotImplementedError
This is the new name?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:40
 +          failure_obj = FailureCrash()
Why the local variables?  Confused.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:37
 +          self.assertNotEqual(failure_obj.result_html_output('foo'), None)
assertTrue would also work.  This seems like not a very useful test.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:69
 +          self.assertRaises(NotImplementedError, failure_obj.message)
I'm not sure the point of this test?

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:553
 +                  output1 = fh1.read()
This doesn't look like it would actually run.  Is it tested?

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:555
 +                  output2 = fh2.read()
This either.  Same typo.

WebKitTools/Scripts/webkitpy/layout_tests/test_types/test_type_base_unittest.py:45
 +  if __name__ == '__main__':
I think PEP8 says 2 blank lines between global blocks.

WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:71
 +          if os.path.exists(filename):
This is redundant.  Just remove the indent.

[Ojan Vafai]

Comment on attachment 65334 [details]
Patch

Nice cleanups.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:553
 +                  output1 = fh1.read()
Shouldn't this be f1? Also, file_handle1 is more readable.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:555
 +                  output2 = fh2.read()
ditto.

WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:71
 +          if os.path.exists(filename):
Don't need this if-statement given the one above.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 65334 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:76
>  +          raise NotImplementedError
> This is the new name?
>

Actually, the old name ("NotImplemented") was a typo, and such an exception does not exist. Good thing we always implmeented the routines :)
 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:40
>  +          failure_obj = FailureCrash()
> Why the local variables?  Confused.

No terribly compelling reason. I'll remove them.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:37
>  +          self.assertNotEqual(failure_obj.result_html_output('foo'), None)
> assertTrue would also work.  This seems like not a very useful test.
> 

It's simply testing that the output isn't null. I don't particularly want to hard-code any specific output here, but I do care that we return something and the function is implemented. I could test for len(x) != 0; any other suggestions?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures_unittest.py:69
>  +          self.assertRaises(NotImplementedError, failure_obj.message)
> I'm not sure the point of this test?
> 

These checks test that the base class functions do in fact raise NotImplement errors; if they didn't (and instead return some "default" text), then the danger is that a subclass wouldn't properly override them.

I don't know if there's a better way to do this in Python, but I'm open to suggestions.

> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:553
>  +                  output1 = fh1.read()
> This doesn't look like it would actually run.  Is it tested?
> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:555
>  +                  output2 = fh2.read()
> This either.  Same typo.
> 

Good catch. I'm not sure why this didn't fall over, but it may not be covered by unit tests (the test coverage on this file is really poor). I'll fix the typos, but I don't want to add more tests for this file specifically as part of this patch, to avoid confusing things (I will be adding more unit tests for this soon).

> WebKitTools/Scripts/webkitpy/layout_tests/test_types/test_type_base_unittest.py:45
>  +  if __name__ == '__main__':
> I think PEP8 says 2 blank lines between global blocks.
>

Right. Not sure how I missed this in check-webkit-style.
 
> WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:71
>  +          if os.path.exists(filename):
> This is redundant.  Just remove the indent.

True. Will do.

[Dirk Pranke]

(In reply to comment #4)
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:553
> >  +                  output1 = fh1.read()
> > This doesn't look like it would actually run.  Is it tested?
> > 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:555
> >  +                  output2 = fh2.read()
> > This either.  Same typo.
> > 
> 
> Good catch. I'm not sure why this didn't fall over, but it may not be covered by unit tests (the test coverage on this file is really poor). I'll fix the typos, but I don't want to add more tests for this file specifically as part of this patch, to avoid confusing things (I will be adding more unit tests for this soon).

Actually I seem to have had writing a unit test for this particular change on my to-do list but lost track of it. It should be easy to add by itself, so I'll add it.

[Dirk Pranke]

Created attachment 65339 [details]
Patch

[Dirk Pranke]

Committed r65953: <http://trac.webkit.org/changeset/65953>