| Summary: | Adding tool to check local changes have been successfully landed via a third-party (such as the commit queue). | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tim 'mithro' Ansell <mithro> | ||||||||||||||||
| Component: | New Bugs | Assignee: | Tim 'mithro' Ansell <mithro> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, alancutter, dpranke, eric, rniwa, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 107408 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Tim 'mithro' Ansell
2013-01-08 18:53:15 PST
Created attachment 181819 [details]
Patch
Comment on attachment 181819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review Do you want this? > Tools/Scripts/webkitpy/tool/steps/landcheck.py:1 > +# Copyright (C) 2010 Google Inc. All rights reserved. 2013. :) > Tools/Scripts/webkitpy/tool/steps/landcheck.py:52 > + raise ScriptError(message="Unable to find committed subversion revision.") > + > + if not scm.working_directory_is_clean(): > + raise ScriptError(message="Uncommited Changes") ScriptError is an error from Executive. We should rename it to ExecutiveError to stop confusion. :) (You're not the first). I would just use Exception or your own error. (In reply to comment #2) > (From update of attachment 181819 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review > > Do you want this? Bah. I misclicked, and typed that in the wrong box. :) Comment on attachment 181819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review This is fantastic, btw. I assume you didn't run the SCM unit tests? (since they're probably broken. You ahve to pass --all to test-webkitpy and then go get a sandwitch. I recommend adding unittests but running only your new ones and not the whole SVN/SCM suite. :( > Tools/Scripts/webkitpy/tool/steps/landcheck.py:57 > + print "Diff with" > + print "git diff %s" % commit_id Do you want this? Comment on attachment 181819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review >> Tools/Scripts/webkitpy/tool/steps/landcheck.py:57 >> + print "git diff %s" % commit_id > > Do you want this? So I ran into a problem, the "scm.rebase_onto_revision()" will become interactive if there is a problem rebasing. This means I can't just run the diff command for you. Any idea on how to solve it? One thought is to do the rebase and check if it succeeded properly, otherwise reset to before the rebase and then output the manual commands you need to run? Seems to get a lot of errors with;
[67/1734] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_not_synced_with_conflict erred:
Traceback (most recent call last):
File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1300, in test_commit_with_message_not_synced_with_conflict
self.assertRaises(ScriptError, scm.commit_with_message, "another test commit", force_squash=True)
File "/usr/lib/python2.7/unittest/case.py", line 471, in assertRaises
callableObj(*args, **kwargs)
File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 377, in commit_with_message
return self.push_local_commits_to_server(username=username, password=password)
File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 462, in push_local_commits_to_server
raise AuthenticationError(SVN.svn_server_host, prompt_for_password=True)
AuthenticationError
Without my patch; Ran 1733 tests in 34.321s FAILED (failures=1, errors=22) With my patch; Ran 1734 tests in 30.736s FAILED (failures=1, errors=22) test-webkitpy (without --all) should have no errors. Sadly scm_unittests.py has been ignored for a *long* time and is likely full of failures. :( Added 1 extra test which doesn't fail. I'll go hit the tests with a stick in another patch. Mike also suggested that I try the most comment case, then if that fails print what a human would need to do. I think this change is a little problematic as-is. I'm not entirely sure what the intent is, as it looks like it's checking two things: 1) that a given bug id has landed, and 2) that if we rebase a git branch against that bug id, then the branch is empty (fully merged), right? Given that this command has side effects (actually rebasing the branch), and isn't actually landing things, calling it "land-check" seems like a bad name. I'm also a bit torn on having a command that only works in git checkouts, but then again, you're not likely to need this at all in an svn checkout. It seems like there has been provisions for git only commands already because of the following;
AbstractDeclarativeCommand.__init__(self, options=options, requires_local_commits=True)
Correct, webkitpy works best with Git, but still supports SVN: https://lists.webkit.org/pipermail/webkit-dev/2012-April/020198.html There are already;
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()
Any better idea for a name then? landed-rebase? compare-to-landed? is your intent to actually rebase as well, or is that just a (necessary?) side effect of checking to see if the branch is merged? Its just a side effect of doing the diff. It also means that any uncommitted changes are easier to rebase onto HEAD. I'm not wild about the side effects. It seems like you can try doing a rebase plus a diff, and if you either get a diff or run into rebasing problems, you know the branch is not fully merged. Either way, abort and do a reset back to the previous branch point. If you took that path, then something like 'check-merged' might be a good name. The original idea was to make sure that I could delete a local git branch because everything on it has been merged successfully. Can you think of a better way to do this? (In reply to comment #19) > The original idea was to make sure that I could delete a local git branch because everything on it has been merged successfully. > Right, doesn't my idea still let you know if it's safe to do that? > Can you think of a better way to do this? Unfortunately, no. I don't know of a good way to do this w/ git either, given the way we land changes. Created attachment 183322 [details]
Patch - Have yet to finish the tests, but would like a review anyway.
Attachment 183322 [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/tool/steps/haslanded.py:51: [HasLanded.strip_change_log] Passing unexpected keyword argument 'flags' in function call [pylint/E1123] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 183322 [details] Patch - Have yet to finish the tests, but would like a review anyway. View in context: https://bugs.webkit.org/attachment.cgi?id=183322&action=review > Tools/ChangeLog:3 > + Adding tool to check your local changes where successfully landed. please go into a bit more detail in your changelogs, so that readers can understand what's going on. I wouuld add the name of the new webkit-patch command, and that it is a new webkit-patch command, at least :). I would also describe in a bit of detail what the command is doing. > Tools/Scripts/webkitpy/common/checkout/diff_parser.py:61 > + ("^index [0-9a-f]*\.\.[0-9a-f]*", lambda matched: "===================================================================\n"), why was this change needed? > Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130 > + # revision. You don't have to limit yourself to 80 cols, so you could probably put this comment on one line. > Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:132 > + for cmt in sorted(self.comments(), reverse=True): webkit style is to not use abbreviated variable names unless they're absolutely common and clear. Even I don't think 'cmt' makes it over that bar. In addition, it's pretty idiomatic, when iterating over a plural, to use the singular as the variable name (e.g., for comment in comments). > Tools/Scripts/webkitpy/tool/steps/haslanded.py:70 > + # Now this is there it gets complicated, we need to compare out diff to the diff at landed_revision. typo: "our" instead of "out"? > Tools/Scripts/webkitpy/tool/steps/haslanded.py:88 > + ["interdiff", landed_file.name, local_file.name], decode_output=False) interdiff? what's interdiff? So, it looks like you've abandoned the 'merge the branch and see if we still have a diff' approach, and now you're attempting to see if the local diff basically matches the diff that landed? Does that actually work in practice? I assume that's what this interdiff thing helps with? Comment on attachment 183322 [details] Patch - Have yet to finish the tests, but would like a review anyway. View in context: https://bugs.webkit.org/attachment.cgi?id=183322&action=review >> Tools/ChangeLog:3 >> + Adding tool to check your local changes where successfully landed. > > please go into a bit more detail in your changelogs, so that readers can understand what's going on. I wouuld add the name of the new webkit-patch command, and that it is a new webkit-patch command, at least :). I would also describe in a bit of detail what the command is doing. Done. >> Tools/Scripts/webkitpy/common/checkout/diff_parser.py:61 >> + ("^index [0-9a-f]*\.\.[0-9a-f]*", lambda matched: "===================================================================\n"), > > why was this change needed? That is a good question, I'm trying to figure out why my diffs have a different format to the output of "git diff". diff --git a/Tools/Scripts/webkitpy/tool/steps/haslanded.py b/Tools/Scripts/webkitpy/tool/steps/haslanded.py new file mode 100644 index 0000000000000000000000000000000000000000..102b7890d5e029fb1c07c0a1ae1017813a12fee8 >> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130 >> + # revision. > > You don't have to limit yourself to 80 cols, so you could probably put this comment on one line. Done >> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:132 >> + for cmt in sorted(self.comments(), reverse=True): > > webkit style is to not use abbreviated variable names unless they're absolutely common and clear. Even I don't think 'cmt' makes it over that bar. In addition, it's pretty idiomatic, when iterating over a plural, to use the singular as the variable name (e.g., for comment in comments). Done. >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:70 >> + # Now this is there it gets complicated, we need to compare out diff to the diff at landed_revision. > > typo: "our" instead of "out"? Done. >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88 >> + ["interdiff", landed_file.name, local_file.name], decode_output=False) > > interdiff? what's interdiff? > > So, it looks like you've abandoned the 'merge the branch and see if we still have a diff' approach, and now you're attempting to see if the local diff basically matches the diff that landed? Does that actually work in practice? I assume that's what this interdiff thing helps with? interdiff is part of the standard GNU diff tools; NAME interdiff - show differences between two diff files SYNOPSIS interdiff [[-p n] | [--strip-match=n]] [[-U n] | [--unified=n]] [[-d PAT] | [--drop-context=PAT]] [[-q] | [--quiet]] [[-z] | [--decompress]] [[-b] | [--ignore-space-change]] [[-B] | [--ignore-blank-lines]] [[-i] | [--ignore-case]] [[-w] | [--ignore-all-space]] [[--interpolate] | [--combine] | [--flip]] [--no-revert-omitted] diff1 diff2 interdiff {[--help] | [--version]} DESCRIPTION interdiff creates a unified format diff that expresses the difference between two diffs. The diffs must both be relative to the same files. For best results, the diffs must have at least three lines of context. Yes, I've abandoned the merge and diff approach and now comparing the two diff files. I hope to add more features to this in the future like "Looks like the diff partially landed, do you want to rebase onto it?" Created attachment 183331 [details]
Patch
Attachment 183331 [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/tool/steps/haslanded.py:51: [HasLanded.strip_change_log] Passing unexpected keyword argument 'flags' in function call [pylint/E1123] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 183331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183331&action=review > Tools/ChangeLog:6 > + This command compares the finally commit patch to the changes which nit: commit -> committed. This is actually a bit more detail than I would've written :). I'd probably just have put something like "was successfully landed by comparing the local diff to what was actually committed". Note that I'm only being so picky here because you're new to the project and I'm trying to give some guidelines; I hope this is helpful. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:88 > + ["interdiff", landed_file.name, local_file.name], decode_output=False) looks like interdiff is only available on my ubuntu precise box by default (not mac or win), so you might want to trap this in case the commands' not available and log something useful. This approach seems reasonable and I'm fine w/ you landing something along these lines to see if it actually works and is useful. Needs at least some minimal testing of the new command, and you need to figure out what's up with the change to the diff_parser before you get an r+, though. Comment on attachment 183331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183331&action=review >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88 >> + ["interdiff", landed_file.name, local_file.name], decode_output=False) > > looks like interdiff is only available on my ubuntu precise box by default (not mac or win), so you might want to trap this in case the commands' not available and log something useful. As the moment it'll raise an error when interdiff isn't found. > > This approach seems reasonable and I'm fine w/ you landing something along these lines to see if it actually works and is useful. I've had quite a few people say they liked the idea of this patch locally, so I think we'll find it gets used quite a bit. > Needs at least some minimal testing of the new command, Adding now. > and you need to figure out what's up with the change to the diff_parser before you get an r+, though. The diff_parser change was caused by create_patch passing --full-index to diff. I've added two test cases to diff_parser to make sure both types of diffs convert successfully. Created attachment 183706 [details]
Patch
Attachment 183706 [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/tool/steps/haslanded_unittest.py:194: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292: trailing whitespace [pep8/W291] [5]
Total errors found: 10 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 183727 [details]
Patch
Attachment 183727 [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/tool/steps/haslanded_unittest.py:194: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292: trailing whitespace [pep8/W291] [5]
Total errors found: 10 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Hey guys, Could I get another review on this patch? I'm unsure the correct way to test the run method, should I just use the MockExecutive? Tim I think what's blocking me from reviewing this patch is understanding the use case. I worry that we haven't achieved sufficent buy-in from the relevant parties here. Namely Dirk Pranke, Adam and myself. Perhaps its easiest for you and Dirk to have a heart-to-heart over irc, or you and I can? Mostly I want to understand why we want this? This makes it so that git users know that their git svn dcommit (aka webkit-patch land) succeeded? I personally only webkit-patch land about once every 6 months. :) Otherwise I use the commit-queue for all my patches, so I'm probably not your target audience. :) Or maybe this is a use case that I haven't even conceived of? This patch is useful for anyone who lands patches via the commit-queue rather than landing using git svn dcommit/svn commit. (Also works in any case where someone else land patches for you). It makes sure your current local changes match the changes that were landed and they didn't accidently land an old patch. This allows you to then discard your local changes without fear of accidentally missing something. It should now work for both svn users and git users. Git users will probably use it more as they are more likely to have lots of local changes being sent out for review. I've updated the title to make more sense. Ah! I see! So this is not something we would add to "land" but rather just a separate command to verify "did this ever get committed" to a local commit? I assume this only works if provided a bug id. :) It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct? (In reply to comment #37) > I assume this only works if provided a bug id. :) It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct? It looks up the bugid in the Changelog (in the same way other commands do) or can be provided one on the commandline. (In reply to comment #36) > Ah! I see! So this is not something we would add to "land" but rather just a separate command to verify "did this ever get committed" to a local commit? Yes, correct! It basically is verifying that you can throw away your local changes because they have already been committed via an alternative route. (In reply to comment #37) > I assume this only works if provided a bug id. :) It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct? Yes, that is correct. Comment on attachment 183727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183727&action=review I'm basically okay with this patch and approach. You should fix (or suppress) the style warnings; otherwise, I'm just looking at minor issues. > Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:136 > + # If we don't find a revision, return None nit: this comment doesn't add much. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:69 > + # Now this is there it gets complicated, we need to compare our diff to the diff at landed_revision. typo: "there" -> "where"? > Tools/Scripts/webkitpy/tool/steps/haslanded.py:84 > + ["interdiff", diff1_patch.name, diff2_patch.name], decode_output=False) I'm still not fond of just raising an uncaught exception if interdiff isn't available. Tools shouldn't just return hard-to-follow stack traces to the user. Can you catch this somewhere and print a more helpful message? > Tools/Scripts/webkitpy/tool/steps/haslanded.py:89 > + return True Should we log some sort of a message in this case rather than silently succeeding? > Tools/Scripts/webkitpy/tool/steps/haslanded.py:118 > + self._tool.scm().discard_local_changes() Do you want this line? Comment on attachment 183727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183727&action=review >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:118 >> + self._tool.scm().discard_local_changes() > > Do you want this line? Whoops, never mind, wasn't thinking about the fact that you told it to discard the changes :). Created attachment 183866 [details]
Patch
Attachment 183866 [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/tool/steps/haslanded_unittest.py:194: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292: trailing whitespace [pep8/W291] [5]
Total errors found: 10 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Friendly poke. The style failures are caused by the tests which have patches in """ strings and thus need the extra space. Comment on attachment 183866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183866&action=review > Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130 > + r = re.compile("Committed r(?P<svn_revision>\d+)") It's slightly sad that we don't keep this string in the same place as it's generated, but OK. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:46 > + @staticmethod Why does everyone love @staticmethod so much? Am I the only hater? :) Statics are impossible to mock, short of replacing them on the actual object. You can't even subclass to mock out a static. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:49 > + convert = diff_parser.get_diff_converter(lines[0]) We don't generally use "get" in getters in WebKit, but obviously this one already exists. Probably imported from some chromium code. :) > Tools/Scripts/webkitpy/tool/steps/haslanded.py:54 > + lines = StringIO.StringIO(diff).readlines() Does readlines return you a generator? Can one just write: for line in StringIO.StringIO(diff)? > Tools/Scripts/webkitpy/tool/steps/haslanded.py:58 > + indexline = re.match("^Index: ([^\\n]*/)?([^/\\n]*)$", line) It's nice to put constants like these somewhere easily identifiable/reusable (like on the SVN object?), but if this is the only place we need this regexp then that's OK. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:72 > + @staticmethod > + def diff_diff(diff1, diff2, diff1_suffix, diff2_suffix, executive=None): > + # Now this is where it gets complicated, we need to compare our diff to the diff at landed_revision. > + diff1_patch = tempfile.NamedTemporaryFile(suffix=diff1_suffix + '.patch') > + diff1_patch.write(diff1) > + diff1_patch.flush() Here again, @staticmethod makes testing/mocking more difficult. FileSystem has a temporary file method iirc, making it possible to test this method with a MockFileSystem instead of a real one. FileSystem also has a write_binary_file helper function which does a full write. :) > Tools/Scripts/webkitpy/tool/steps/haslanded.py:81 > + # Diff the two diff's together... > + if not executive: > + executive = Executive() If we're adidng new code, we should require an Executive. Our current testing strategy tries to leverage our central mocks as much as possible. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:99 > + raise ScriptError("Unable to find landed message in associated bug.") ScriptError is an Executive-only thing and really should be renamed ExecutiveError. It does have some special handling in webkit-patch, but it feels wrong to reuse-it here. Callers will expect it to have properties which make no sense in this case. I would just use Exception here. > Tools/Scripts/webkitpy/tool/steps/haslanded.py:114 > + pretty_diff_file = self._show_pretty_diff(diff_diff) Use the with construction, and then you don't have to close. :) > Tools/Scripts/webkitpy/tool/steps/haslanded.py:117 > + diff_correct = self._tool.user.confirm("Can I discard local changes?") I believe the grammar nazis would tell us it's "may I" :) >> Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292 >> + > > trailing whitespace [pep8/W291] [5] Is there some way to tell pep8 to ignore this warning for these """? Comment on attachment 183866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183866&action=review >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:46 >> + @staticmethod > > Why does everyone love @staticmethod so much? Am I the only hater? :) Statics are impossible to mock, short of replacing them on the actual object. You can't even subclass to mock out a static. Change to @classmethod >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:49 >> + convert = diff_parser.get_diff_converter(lines[0]) > > We don't generally use "get" in getters in WebKit, but obviously this one already exists. Probably imported from some chromium code. :) Okay :) >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:54 >> + lines = StringIO.StringIO(diff).readlines() > > Does readlines return you a generator? Can one just write: for line in StringIO.StringIO(diff)? Done, it's leftover from when I was doing things with lines first. >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:58 >> + indexline = re.match("^Index: ([^\\n]*/)?([^/\\n]*)$", line) > > It's nice to put constants like these somewhere easily identifiable/reusable (like on the SVN object?), but if this is the only place we need this regexp then that's OK. I think it's okay for now. Ideally we'd replace this function with something in the SCM object which knows how to discard parts of a patch properly... >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:114 >> + pretty_diff_file = self._show_pretty_diff(diff_diff) > > Use the with construction, and then you don't have to close. :) Done. >> Tools/Scripts/webkitpy/tool/steps/haslanded.py:117 >> + diff_correct = self._tool.user.confirm("Can I discard local changes?") > > I believe the grammar nazis would tell us it's "may I" :) Done. >>> Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292 >>> + >> >> trailing whitespace [pep8/W291] [5] > > Is there some way to tell pep8 to ignore this warning for these """? You can use # nopep8 to stop pep8 from apply to a line, but that would break the patches... Created attachment 184417 [details]
Patch
Attachment 184417 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/net/bugzilla/bug.py', u'Tools/Scripts/webkitpy/common/net/bugzilla/bug_unittest.py', u'Tools/Scripts/webkitpy/tool/commands/upload.py', u'Tools/Scripts/webkitpy/tool/steps/__init__.py', u'Tools/Scripts/webkitpy/tool/steps/haslanded.py', u'Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:194: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283: trailing whitespace [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292: trailing whitespace [pep8/W291] [5]
Total errors found: 10 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 184417 [details]
Patch
OK.
Comment on attachment 184417 [details] Patch Rejecting attachment 184417 [details] from commit-queue. New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Full output: http://queues.webkit.org/results/16076699 The flakiness must end! Comment on attachment 184417 [details] Patch Clearing flags on attachment: 184417 Committed r140674: <http://trac.webkit.org/changeset/140674> All reviewed patches have been landed. Closing bug. The test added by this patch is failing on bots: http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/6087/steps/webkitpy-test/logs/stdio [1623/1639] webkitpy.tool.steps.haslanded_unittest.HasLandedTest.test_run erred: Traceback (most recent call last): File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py", line 134, in test_run HasLanded.diff_diff(diff1, diff1_add_line, '', 'add-line'), File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/tool/steps/haslanded.py", line 84, in diff_diff ["interdiff", diff1_patch.name, diff2_patch.name], decode_output=False) File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 411, in run_command close_fds=self._should_close_fds()) File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 482, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 679, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1228, in _execute_child raise child_exception OSError: [Errno 2] No such file or directory Unfortunately the CQ/EWS no longer run test-webkitpy. They used to,but because we don't parse test-webkitpy output currently, the CQ/EWS have no way to ignore known failures and would fail forever if the tree was red. I suspect the fix here is easy. Lets see if mithro is around #webkit. Skipped the test temporarily in http://trac.webkit.org/changeset/140776. Made the test run on machines with interdiff, in http://trac.webkit.org/changeset/140777 (In reply to comment #58) > Made the test run on machines with interdiff, in http://trac.webkit.org/changeset/140777 'which interdiff' isn't going to work on windows. I guess maybe that'll sort of be okay, but probably only accidentally :). Also, having to call out to a subprocess in a decorator might be kinda slow. We should probably fix this more robustly. |