Bug 36696 – scm_unittest: Get the SVNTest.test_svn_apply() unit test working again

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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

[Chris Jerdonek]

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'

[Adam Barth]

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.

[Eric Seidel (no email)]

(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

[Eric Seidel (no email)]

Comment on attachment 51820 [details]
Proposed patch

Wow, that is subtle.

I would rather that the _reviewer setting be done inside _create_patch though.

[Chris Jerdonek]

(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. :)

[Adam Barth]

(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.

[Eric Seidel (no email)]

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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

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.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 51831 [details]
Proposed patch 2

We'll hope this was cosmic rays.

[WebKit Commit Bot]

Comment on attachment 51831 [details]
Proposed patch 2

Clearing flags on attachment: 51831

Committed r56762: <http://trac.webkit.org/changeset/56762>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.