Summary: | Fix SCM webkitpy unit test failures | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||
Component: | New Bugs | Assignee: | jochen | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dbates, eric, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
jochen
2011-08-06 16:03:54 PDT
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 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. > 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
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 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
Committed r92632: <http://trac.webkit.org/changeset/92632> 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 |