Bug 65823 - Fix SCM webkitpy unit test failures
Summary: Fix SCM webkitpy unit test failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-06 16:03 PDT by jochen
Modified: 2011-08-08 14:25 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.90 KB, patch)
2011-08-06 16:05 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2011-08-08 00:25 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2011-08-08 01:07 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (5.70 KB, patch)
2011-08-08 11:25 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-08-06 16:03:54 PDT
Fix SCM webkitpy unit test failures
Comment 1 jochen 2011-08-06 16:05:35 PDT
Created attachment 103170 [details]
Patch
Comment 2 jochen 2011-08-06 16:08:51 PDT
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
Comment 3 jochen 2011-08-08 00:25:26 PDT
Created attachment 103212 [details]
Patch
Comment 4 jochen 2011-08-08 00:28:49 PDT
Actually, GitTest.test_create_patch can be fixed by using the tracking SCM instead
Comment 5 Adam Barth 2011-08-08 00:32:07 PDT
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.
Comment 6 Adam Barth 2011-08-08 00:33:00 PDT
> 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.
Comment 7 jochen 2011-08-08 00:52:57 PDT
(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.
Comment 8 jochen 2011-08-08 01:07:57 PDT
Created attachment 103218 [details]
Patch
Comment 9 WebKit Review Bot 2011-08-08 01:10:16 PDT
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.
Comment 10 Eric Seidel (no email) 2011-08-08 09:11:35 PDT
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?
Comment 11 jochen 2011-08-08 11:23:37 PDT
(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
Comment 12 jochen 2011-08-08 11:25:29 PDT
Created attachment 103268 [details]
Patch
Comment 13 Adam Barth 2011-08-08 14:04:41 PDT
Committed r92632: <http://trac.webkit.org/changeset/92632>
Comment 14 Adam Barth 2011-08-08 14:05:35 PDT
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.
Comment 15 jochen 2011-08-08 14:25:00 PDT
(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