Bug 65823 – Fix SCM webkitpy unit test failures

• Component: New Bugs
• Status: RESOLVED FIXED

Fix SCM webkitpy unit test failures

Created attachment 103170 [details]
Patch

Comment on attachment 103170 [details]
Patch

SVNTest.test_svn_apply - use the correct time zone matching svn-apply's timezone and make sure the whitespace in the diffs doesn't get swallowed

GitTestWithMock.test_create_patch - update expected result with correct checkout root

GitTest.test_create_patch - remove scm.create_patch only works if there's a underlying svn repository

Git.revisions_changing_file - throw an exception if the path does not exist to match SVN

Created attachment 103212 [details]
Patch

Actually, GitTest.test_create_patch can be fixed by using the tracking SCM instead

[Adam Barth]

Comment on attachment 103212 [details]
Patch

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

Eric should really review this patch.  I've left some questions below.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:207
> +        # raise a script error if path does not exists to match the behavior of  the svn implementation.
> +        if not os.path.exists(path):
> +            raise ScriptError(message="Path %s does not exist." % path)

Are we supposed to use the filesystem abstraction instead of the real filesystem here?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:543
> +            os.environ['TZ'] = oldTZ

Is this correct if the environment lacked a TZ variable before?

This code shouldn't be here at all.  ChangeLog-level concepts should be in checkout.py.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:956
> +        os.chdir(self.tracking_git_checkout_path)

Do we really chdir during these tests?  That's global static state that's scary to twiddle.

[Adam Barth]

> This code shouldn't be here at all.  ChangeLog-level concepts should be in checkout.py.

This comment lost it's context.  It was meant to apply to the test_create_patch code.

(In reply to comment #5)
> (From update of attachment 103212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103212&action=review
> 
> Eric should really review this patch.  I've left some questions below.
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:207
> > +        # raise a script error if path does not exists to match the behavior of  the svn implementation.
> > +        if not os.path.exists(path):
> > +            raise ScriptError(message="Path %s does not exist." % path)
> 
> Are we supposed to use the filesystem abstraction instead of the real filesystem here?

I figured using os.path.exists would be closest to the SVN behavior, as it is the svn binary that chickens out if the path doesn't exist


> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:543
> > +            os.environ['TZ'] = oldTZ
> 
> Is this correct if the environment lacked a TZ variable before?

done

> 
> This code shouldn't be here at all.  ChangeLog-level concepts should be in checkout.py.

even with the additional context I don't understand the comment? test_create_patch isn't ChangeLog level. Do you mean test_svn_apply?

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:956
> > +        os.chdir(self.tracking_git_checkout_path)
> 
> Do we really chdir during these tests?  That's global static state that's scary to twiddle.

actually, the chdir isn't required here. removed.

Created attachment 103218 [details]
Patch

[WebKit Review Bot]

Attachment 103218 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:538:  .has_key() is deprecated, use 'in'  [pep8/W601] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 103218 [details]
Patch

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

Aside from the timezone thing, this change looks great!

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:206
> +        if not os.path.exists(path):

This should be self.filesystem.exists (assuming SCM/Git has a filesystem object?)

>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:538
>> +            has_tz = os.environ.has_key('TZ')
> 
> .has_key() is deprecated, use 'in'  [pep8/W601] [5]

I think you mean 'TZ' in os.environ?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:548
> +            old_tz = os.environ.get('TZ', '')
> +            os.environ['TZ'] = 'PST8PDT'
> +            time.tzset()
> +        changelog_entry = changelog_entry.replace('DATE_HERE', date.today().isoformat())
> +        if hasattr(time, 'tzset'):
> +            if has_tz:
> +                os.environ['TZ'] = old_tz
> +            else:
> +                del os.environ['TZ']
> +            time.tzset()

Really?  This can't be right.  We can't override the timezone in some cleaner way?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:614
> +""" % {'whitespace': ' '}

This is to get around the tools wanting to remove trailing whitespace?

(In reply to comment #10)
> (From update of attachment 103218 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103218&action=review
> 
> Aside from the timezone thing, this change looks great!
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:206
> > +        if not os.path.exists(path):
> 
> This should be self.filesystem.exists (assuming SCM/Git has a filesystem object?)

done

> 
> >> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:538
> >> +            has_tz = os.environ.has_key('TZ')
> > 
> > .has_key() is deprecated, use 'in'  [pep8/W601] [5]
> 
> I think you mean 'TZ' in os.environ?

done

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:548
> > +            old_tz = os.environ.get('TZ', '')
> > +            os.environ['TZ'] = 'PST8PDT'
> > +            time.tzset()
> > +        changelog_entry = changelog_entry.replace('DATE_HERE', date.today().isoformat())
> > +        if hasattr(time, 'tzset'):
> > +            if has_tz:
> > +                os.environ['TZ'] = old_tz
> > +            else:
> > +                del os.environ['TZ']
> > +            time.tzset()
> 
> Really?  This can't be right.  We can't override the timezone in some cleaner way?

I'm no python expert, and timezone support in python seems to be very rudimentary

So I decided to mimick the code svn-apply is using http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm#L1364

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:614
> > +""" % {'whitespace': ' '}
> 
> This is to get around the tools wanting to remove trailing whitespace?

Right. I thought making the whitespaces explicit is less likely to regress than just adding the missing whitespaces back

Created attachment 103268 [details]
Patch

[Adam Barth]

Committed r92632: <http://trac.webkit.org/changeset/92632>

[Adam Barth]

Comment on attachment 103268 [details]
Patch

jochen: I landed your patch, except for the TZ thing, which seems slightly controversial.  Please feel free to open a new bug for the TZ issue.

(In reply to comment #14)
> (From update of attachment 103268 [details])
> jochen: I landed your patch, except for the TZ thing, which seems slightly controversial.  Please feel free to open a new bug for the TZ issue.

Added bug 65877 to track this