Bug 106429 – webkitpy's SCM unit tests fail

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim 'mithro' Ansell]

Fix both working_directory and commit_with_message functions.

[Tim 'mithro' Ansell]

Created attachment 181878 [details]
Patch

[Tim 'mithro' Ansell]

tansell@tansell-z620-l:/fast/chrome/src/third_party/WebKit/Tools/Scripts$ ./test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitSVNTest
Suppressing most webkitpy logging while running unit tests.
[12/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_git_commit erred:                
  Traceback (most recent call last):
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1205, in test_commit_with_message_git_commit
      commit_text = self.scm.commit_with_message("another test commit", git_commit="HEAD^")
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 364, in commit_with_message
      raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.")
  ScriptError: Working copy is modified. Cannot commit individual git_commits.
  
[18/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_git_commit_range erred: 
  Traceback (most recent call last):
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1215, in test_commit_with_message_git_commit_range
      commit_text = self.scm.commit_with_message("another test commit", git_commit="HEAD~2..HEAD")
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 364, in commit_with_message
      raise ScriptError(message="Working copy is modified. Cannot commit individual git_commits.")
  ScriptError: Working copy is modified. Cannot commit individual git_commits.
  
[28/63] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_only_local_commit erred:
  Traceback (most recent call last):
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1225, in test_commit_with_message_only_local_commit
      commit_text = self.scm.commit_with_message("another test commit")
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 368, in commit_with_message
      self._assert_can_squash(working_directory_is_clean)
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 345, in _assert_can_squash
      raise AmbiguousCommitError(num_local_commits, working_directory_is_clean)
  AmbiguousCommitError
  
Ran 63 tests in 34.061s                                                                                                             
FAILED (failures=0, errors=3)


after

$ ./test-webkitpy webkitpy.common.checkout.scm.scm_unittest.GitSVNTest
Suppressing most webkitpy logging while running unit tests.
Ran 63 tests in 36.501s                                                                                                             

OK

[Eric Seidel (no email)]

Comment on attachment 181878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152
> -        return self._run_git(['diff', self.remote_merge_base(), '--no-renames', '--name-only']) == ""
> +        return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == ""

Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted).  You'd have to ask dpranke.

[Tim 'mithro' Ansell]

If I understand this correctly the following changes have occured.

The working_directory_is_clean and clean_working_directory functions now deal with the normal meaning of "working directory", IE stuff which has yet to be committed.

The commit_with_message function has been modify so it does one of the following options (which is what the tests are currently testing for):

If you give it a commit range, then it'll dcommit the git commits referred too.

If you have asked it to squash (via either the force_squash or the webkit-patch.commit-should-always-squash config) then it will squash all yet to dcommitted git commits and anything in the working directory together, then dcommit the result.

Otherwise, it will check there is only a single thing to dcommit, either a single git commit, or stuff in the working directory, and dcommit that.

Does that match what you actually want it to do?

Tim

[Tim 'mithro' Ansell]

(In reply to comment #4)
> If I understand this correctly the following changes have occured.
> 
> The working_directory_is_clean and clean_working_directory functions now deal with the normal meaning of "working directory", IE stuff which has yet to be committed.

To be clear, I actually mean changes which have yet to be committed to EITHER subversion OR git.

[Dirk Pranke]

Comment on attachment 181878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152
>> +        return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == ""
> 
> Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted).  You'd have to ask dpranke.

Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change.

[Adam Barth]

Comment on attachment 181878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review

>>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152
>>> +        return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == ""
>> 
>> Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted).  You'd have to ask dpranke.
> 
> Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change.

Yeah, the problem is that if we have local commits (i.e., HEAD points to something we've committed locally), then clean_working_directory will leave those commits around instead of blowing them away.

This happens all the time on the commit-queue because the way it lands patches is to first commit them locally and then dcommit them to svn.  If the dcommit fails, it will have the local commit.  If clean doesn't blow the local commit away, it will bork the queue until it does its big working copy reset.

[Tim 'mithro' Ansell]

Comment on attachment 181878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181878&action=review

>>>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:152
>>>> +        return self._run_git(['diff', 'HEAD', '--no-renames', '--name-only']) == ""
>>> 
>>> Its possible this change was intentional, but the tests wer never updated to reflect it (since they were slow/busted).  You'd have to ask dpranke.
>> 
>> Right, this change was intentional and we need to fix the tests. The issue comes up when the CQ is ahead of the remote for one reason or another. I think abarth actually made this change.
> 
> Yeah, the problem is that if we have local commits (i.e., HEAD points to something we've committed locally), then clean_working_directory will leave those commits around instead of blowing them away.
> 
> This happens all the time on the commit-queue because the way it lands patches is to first commit them locally and then dcommit them to svn.  If the dcommit fails, it will have the local commit.  If clean doesn't blow the local commit away, it will bork the queue until it does its big working copy reset.

Your change is redefining "working_directory" to be something very unexpected (at least for experienced git users). It seems like you also want a "reset to remote_merge_base" type thing?

[Eric Seidel (no email)]

Yes, the SCM API name should likely change.  SCM was "designed" to be an abstraction around the current checkout's scm system, and is thus a strange compromise between svn and git.

[Tim 'mithro' Ansell]

(In reply to comment #9)
> Yes, the SCM API name should likely change.  SCM was "designed" to be an abstraction around the current checkout's scm system, and is thus a strange compromise between svn and git.

You already have the ability to discard local commits using Git.discard_local_commits?

Maybe we should change the caller to do a "if self.supports_local_commits(): self.discard_local_commits()"?

[Dirk Pranke]

You should probably check out the previous discussion in bug 104032 (and bug 104198 that we filed about the fact that the method name is now confusing and/or wrong ;).

[Tim 'mithro' Ansell]

The right path forward I think is to revert 104032 and use the following to functions;

    def ensure_clean_working_directory(self, force_clean):
        if self.working_directory_is_clean():
            return
        if not force_clean:
            print self.run(self.status_command(), error_handler=Executive.ignore_error, cwd=self.checkout_root)
            raise ScriptError(message="Working directory has modifications, pass --force-clean or --no-clean to continue.")
        _log.info("Cleaning working directory")
        self.clean_working_directory()



    def ensure_no_local_commits(self, force):
        if not self.supports_local_commits():
            return
        commits = self.local_commits()
        if not len(commits):
            return
        if not force:
            _log.error("Working directory has local commits, pass --force-clean to continue.")
            sys.exit(1) #### WHY!? Oh god why!?
        self.discard_local_commits()



??

[Tim 'mithro' Ansell]

Scripts/webkitpy/tool/steps/cleanworkingdirectory.py then should be change to something like "resettoupstream" or something?

[Tim 'mithro' Ansell]

Created attachment 182099 [details]
Patch

[Adam Barth]

Comment on attachment 182099 [details]
Patch

Won't this patch break the commit-queue as described above?

[Adam Barth]

I don't really understand what problem you're trying to solve.  "Fix both working_directory and commit_with_message functions" doesn't mean anything to me and your change log does not provide any additional information.

[Dirk Pranke]

(In reply to comment #16)
> I don't really understand what problem you're trying to solve.  "Fix both working_directory and commit_with_message functions" doesn't mean anything to me and your change log does not provide any additional information.

I think the subject refers to the failing tests and then the belief that the working_directory_is_clean() function was doing the wrong thing.

[Adam Barth]

Perhaps we should retitle the bug them?  These functions are working as intended (even if they could benefit from more git-centric names).

[Dirk Pranke]

sure :).

[Tim 'mithro' Ansell]

Created attachment 183099 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 183099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183099&action=review

OK.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:129
> +        executive = executive or Executive()

This is still a FIXME, but way better, thank you!

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1551
> +    maxDiff = None

Was this intentional?

[Tim 'mithro' Ansell]

Created attachment 183346 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183346 [details]
Patch

Clearing flags on attachment: 183346

Committed r140089: <http://trac.webkit.org/changeset/140089>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.