Bug 36696 - scm_unittest: Get the SVNTest.test_svn_apply() unit test working again
Summary: scm_unittest: Get the SVNTest.test_svn_apply() unit test working again
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-27 01:35 PDT by Chris Jerdonek
Modified: 2010-03-29 22:41 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (3.83 KB, patch)
2010-03-27 01:48 PDT, Chris Jerdonek
eric: review-
Details | Formatted Diff | Diff
Proposed patch 2 (3.29 KB, patch)
2010-03-27 09:49 PDT, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-03-27 01:35:45 PDT
This was first stated here:

https://bugs.webkit.org/show_bug.cgi?id=36257

The explanation for the failure was given here (extra spaces needed and patch.reviewer() not evaluating correctly):

https://bugs.webkit.org/show_bug.cgi?id=36257#c3
Comment 1 Chris Jerdonek 2010-03-27 01:48:12 PDT
Created attachment 51820 [details]
Proposed patch

After this patch, this is the only failing "test-webkitpy --all" unit test:

FAIL: test_sheriff_bot (webkitpy.tool.commands.sheriffbot_unittest.SheriffBotTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/chris_g4/dev/apple/WebKit-git/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py", line 47, in test_sheriff_bot
    self.assert_queue_outputs(SheriffBot(), work_item=mock_work_item, expected_stderr=expected_stderr)
  File "/Users/chris_g4/dev/apple/WebKit-git/WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py", line 78, in assert_queue_outputs
    expected_stderr=expected_stderr.get("begin_work_queue", ""))
  File "/Users/chris_g4/dev/apple/WebKit-git/WebKitTools/Scripts/webkitpy/common/system/outputcapture.py", line 60, in assert_outputs
    testcase.assertEqual(stderr_string, expected_stderr)
AssertionError: 'CAUTION: sheriff-bot will discard all local changes in "/private/tmp"\nRunning WebKit sheriff-bot.\n' != 'CAUTION: sheriff-bot will discard all local changes in "/Users/chris_g4/dev/apple/WebKit-git"\nRunning WebKit sheriff-bot.\n'
Comment 2 Adam Barth 2010-03-27 09:11:13 PDT
Looks reasonable, but I'll let Eric review it properly.

The webkitpy.tool.commands.sheriffbot_unittest.SheriffBotTest is because the SCM unit tests aren't restoring the current working directory properly.
Comment 3 Eric Seidel (no email) 2010-03-27 09:14:29 PDT
(In reply to comment #2)
> Looks reasonable, but I'll let Eric review it properly.
> 
> The webkitpy.tool.commands.sheriffbot_unittest.SheriffBotTest is because the
> SCM unit tests aren't restoring the current working directory properly.

Actually, I suspect it's due to this line:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py#L42
which should not be using os.getcwd() and instead using:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/tool/mocktool.py#L328
Comment 4 Eric Seidel (no email) 2010-03-27 09:16:14 PDT
Comment on attachment 51820 [details]
Proposed patch

Wow, that is subtle.

I would rather that the _reviewer setting be done inside _create_patch though.
Comment 5 Chris Jerdonek 2010-03-27 09:16:43 PDT
(In reply to comment #2)
> Looks reasonable, but I'll let Eric review it properly.

Thanks for the comment, Adam!  I know you guys had some FIXME's around the Attachment part of that unit test, so getting the reviewer part to work was admittedly a bit hackish.  But at least it works now. :)
Comment 6 Adam Barth 2010-03-27 09:22:29 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Looks reasonable, but I'll let Eric review it properly.
> > 
> > The webkitpy.tool.commands.sheriffbot_unittest.SheriffBotTest is because the
> > SCM unit tests aren't restoring the current working directory properly.
> 
> Actually, I suspect it's due to this line:
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py#L42
> which should not be using os.getcwd() and instead using:
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/tool/mocktool.py#L328

The test passes fine without the --all flag, so I think it's some interaction between the SCM unit tests, that line, and the cwd.
Comment 7 Eric Seidel (no email) 2010-03-27 09:24:38 PDT
Yes.  I remember diagnosing it when I added the fake_checkout_root stuff.  I can't remember the cause now.  I think its the style code which ends up changing the cwd.  In either case, teh fake_checkout_root fix will work.  We shouldn't be using os.getcwd anywhere really.
Comment 8 Chris Jerdonek 2010-03-27 09:28:04 PDT
(In reply to comment #4)
> (From update of attachment 51820 [details])
> Wow, that is subtle.
> 
> I would rather that the _reviewer setting be done inside _create_patch though.

I believe that's what I had tried first, but then it broke like 9 other tests...  That's partly what I meant by hackish.  :)

Per the FIXME's I saw at the top near the import statement and at the end of the _create_patch() method, this all needs to get refactored a bit anyways (e.g. not to use the Attachment class).  I figured that can wait until later since it's a bigger change -- at which point your comment will get addressed.  In the meantime I just wanted to get the test passing.
Comment 9 Chris Jerdonek 2010-03-27 09:30:04 PDT
(In reply to comment #7)
> Yes.  I remember diagnosing it when I added the fake_checkout_root stuff.  I
> can't remember the cause now.  I think its the style code which ends up
> changing the cwd.  In either case, teh fake_checkout_root fix will work.  We
> shouldn't be using os.getcwd anywhere really.

Yeah, I am using mock_os-type stuff in some of my new tests.
Comment 10 Chris Jerdonek 2010-03-27 09:32:42 PDT
(In reply to comment #8)
> (In reply to comment #4)
> > (From update of attachment 51820 [details] [details])
> > Wow, that is subtle.
> > 
> > I would rather that the _reviewer setting be done inside _create_patch though.
> 
> I believe that's what I had tried first, but then it broke like 9 other
> tests...  That's partly what I meant by hackish.  :)

Let me give it another shot, though.  I might have done it wrong the first time since I might not have been using the Committer class, which I found out later I needed to use.
Comment 11 Chris Jerdonek 2010-03-27 09:49:21 PDT
Created attachment 51831 [details]
Proposed patch 2

Yeah, just needed to use the Committer class in _create_patch().  Thanks for that.

By the way, something seems a bit hard to use about the Attachment class in that the constructor accepts an attachment dictionary rather than a Committer instance, etc.  It seems like it might be better if the constructor were more traditional and a factory method were used for the dictionary route.
Comment 12 Eric Seidel (no email) 2010-03-29 19:57:29 PDT
Comment on attachment 51831 [details]
Proposed patch 2

LGTM.  We should consider making some sort of note in teh file about the intentional trailing spaces.
Comment 13 WebKit Commit Bot 2010-03-29 20:17:51 PDT
Comment on attachment 51831 [details]
Proposed patch 2

Rejecting patch 51831 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-javascriptcore-tests']" exit_code: 139
Last 500 characters of output:
rojects/CommitQueue/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Script-933457200EBFDC3F00B80894.sh

PhaseScriptExecution /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Script-5D29D8BE0E9860B400C3D2D0.sh
    cd /Users/eseidel/Projects/CommitQueue/JavaScriptCore
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Script-5D29D8BE0E9860B400C3D2D0.sh
Compiling jsc failed!

Full output: http://webkit-commit-queue.appspot.com/results/1603081
Comment 14 Eric Seidel (no email) 2010-03-29 22:10:28 PDT
Comment on attachment 51831 [details]
Proposed patch 2

We'll hope this was cosmic rays.
Comment 15 WebKit Commit Bot 2010-03-29 22:41:31 PDT
Comment on attachment 51831 [details]
Proposed patch 2

Clearing flags on attachment: 51831

Committed r56762: <http://trac.webkit.org/changeset/56762>
Comment 16 WebKit Commit Bot 2010-03-29 22:41:36 PDT
All reviewed patches have been landed.  Closing bug.