Bug 38862 - svn-apply: Eliminate the "copiedFiles" hash
Summary: svn-apply: Eliminate the "copiedFiles" hash
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-05-10 12:53 PDT by Chris Jerdonek
Modified: 2010-05-11 22:10 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (2.23 KB, patch)
2010-05-10 18:55 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-05-10 12:53:07 PDT
This report is to do a few clean-ups in svn-apply:

(1) Eliminate use of %copiedFiles (it's not necessary)
(2) Address the FIXME to use exitStatus() in gitKnowsOfFile()
Comment 1 Chris Jerdonek 2010-05-10 18:55:55 PDT
Created attachment 55638 [details]
Proposed patch
Comment 2 Eric Seidel (no email) 2010-05-10 19:06:29 PDT
Comment on attachment 55638 [details]
Proposed patch

Do we know this is covered properly with tests?

I woudl have written this:
+    my $exitStatus = exitStatus($?);
+    return $exitStatus == 0;

as just:
return exitStatus($?) == 0;

Then again, That should really use system instead of `` since it doesn't need/want the output.
Comment 3 Chris Jerdonek 2010-05-10 19:38:51 PDT
(In reply to comment #2)

Thanks, Eric!

> (From update of attachment 55638 [details])
> Do we know this is covered properly with tests?

Unfortunately, this area of svn-apply is not covered by unit tests.  I did do some manual tests though.  Now that you mention it, I'll also run "test-webkitpy --all" before landing to make sure.

> I woudl have written this:
> +    my $exitStatus = exitStatus($?);
> +    return $exitStatus == 0;
> 
> as just:
> return exitStatus($?) == 0;

Yes, thanks.  I suppose I left it as it appears now because it makes it easier to later add a "print($exitStatus)" line for debug purposes.
Comment 4 Eric Seidel (no email) 2010-05-10 19:45:18 PDT
Eeek!  No tests!?  Why should I r+ this then? :(((
Comment 5 Chris Jerdonek 2010-05-11 17:18:19 PDT
(In reply to comment #4)
> Eeek!  No tests!?  Why should I r+ this then? :(((

That question seems rhetorical so I won't answer it.  (Let me know if it's not.)

In terms of tests, this change doesn't add to or change any functionality, so I didn't add any tests.  Is it a requirement for patches that refactor non-unit-tested code to create tests?  I was just trying to simplify the existing code without doing too much extra work.  It looks like it could be a bigger project to add a decent test harness for testing higher-level Perl scm functionality (something I've made a lot of headway on, incidentally, via the parse-related subroutines in VCSUtils.pm and bug 34033 which was a keystone).
Comment 6 Eric Seidel (no email) 2010-05-11 20:24:49 PDT
In general it's *strongly encouraged* to test code when touching it.  In times when I've thought it would be a lot of work to unit test something in the python code I've found that it was less work than I thought, and totally worth the effort.
Comment 7 Eric Seidel (no email) 2010-05-11 20:25:12 PDT
That said, my r+ still stands... I'm just trying to lay on the guilt real thick...
Comment 8 Chris Jerdonek 2010-05-11 22:10:50 PDT
Comment on attachment 55638 [details]
Proposed patch

Clearing flags on attachment: 55638

Committed r59209: <http://trac.webkit.org/changeset/59209>
Comment 9 Chris Jerdonek 2010-05-11 22:10:57 PDT
All reviewed patches have been landed.  Closing bug.