Bug 92913

Created attachment 155934 [details] work in progress

Created attachment 155935 [details] Added the forgotten perfalizer.py

Created attachment 156847 [details] work in progress 3

Created attachment 157075 [details] work in progress 4

Created attachment 157081 [details] work in progress 5

Created attachment 157087 [details] Adds the initial implementation of perfalizer

Unfortunately, there isn't much we can test here because it uses a lot of methods that themselves cannot be tested.

Comment on attachment 157087 [details] Adds the initial implementation of perfalizer View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review Disclaimer: I don't really know the IRCbot code at all, so some of these questions might be a bit off-base. Also, I'd want to give a bit more thought to if/how we can test this. I understand the comment about this being untestable, but python's static checking is pretty weak. We might want to consider at least putting in something that runs lint-webkitpy over this to catch more errors. > Tools/Scripts/webkitpy/common/system/filesystem.py:80 > + You should probably add a routine to filesystem_mock as well ... > Tools/Scripts/webkitpy/tool/commands/perfalizer.py:49 > + def _copy_build_product_without_patch(self, postfix): 'postfix' isn't a great name here; at the least it should probably be "suffix", but even better would be something like "directory_suffix" or "build_dir_suffix" > Tools/Scripts/webkitpy/tool/commands/perfalizer.py:52 > + self._build_path = filesystem.dirname(self._port._build_path()) It's a bit confusing that self._build_path != self._port._build_path() ; maybe pick a different name for the former? This is especially true since you end up adding the configuration dirs back on down below. > Tools/Scripts/webkitpy/tool/commands/perfalizer.py:58 > + filesystem.join(self._build_path_without_patch, configuration)) I wonder if something like rsync -a --delete would be much of a win here over rm + cp. I suppose it depends on how often you'll be building ... > Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64 > + if not self._patch.committer() and not self._patch.attacher().can_commit: Should we have a comment here about why we need a committer here? > Tools/Scripts/webkitpy/tool/commands/perfalizer.py:135 > + self.run_webkit_patch(command) are this and the functions below used anywhere and/or needed? They seem unrelated and possibly just copy&paste from a different command?

Comment on attachment 157087 [details] Adds the initial implementation of perfalizer View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review >> Tools/Scripts/webkitpy/common/system/filesystem.py:80 >> + > > You should probably add a routine to filesystem_mock as well ... Possibly but filesystem_mock doesn't seem to have methods not used in tests so we can probably wait 'til we need one in tests. >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:52 >> + self._build_path = filesystem.dirname(self._port._build_path()) > > It's a bit confusing that self._build_path != self._port._build_path() ; maybe pick a different name for the former? This is especially true since you end up adding the configuration dirs back on down below. Oh oops, yeah, I should have named it self._build_directory. >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:58 >> + filesystem.join(self._build_path_without_patch, configuration)) > > I wonder if something like rsync -a --delete would be much of a win here over rm + cp. I suppose it depends on how often you'll be building ... Maybe but copytree is so much slower that the time spent in rmtree doesn't really matter. >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64 >> + if not self._patch.committer() and not self._patch.attacher().can_commit: > > Should we have a comment here about why we need a committer here? What kind of comment are we going to add? >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:135 >> + self.run_webkit_patch(command) > > are this and the functions below used anywhere and/or needed? They seem unrelated and possibly just copy&paste from a different command? Yes. Otherwise PatchAnalysisTask is going to blow up.

(In reply to comment #9) > (From update of attachment 157087 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review > > >> Tools/Scripts/webkitpy/common/system/filesystem.py:80 > >> + > > > > You should probably add a routine to filesystem_mock as well ... > > Possibly but filesystem_mock doesn't seem to have methods not used in tests so we can probably wait 'til we need one in tests. > Sure. > >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64 > >> + if not self._patch.committer() and not self._patch.attacher().can_commit: > > > > Should we have a comment here about why we need a committer here? > > What kind of comment are we going to add? > Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ...

Comment on attachment 157087 [details] Adds the initial implementation of perfalizer View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review >>>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64 >>>> + if not self._patch.committer() and not self._patch.attacher().can_commit: >>> >>> Should we have a comment here about why we need a committer here? >> >> What kind of comment are we going to add? > > Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ... Oh, that's merely because I don't trust those patches to be ran on my Mac mini at the moment. I'm not sure if adding a comment like "rniwa doesn't trust patches submitted by non-committers" is any more helpful than the code itself given that nobody else is going to run this thing (surely, we can't have two IRC bots named perfalizer).

(In reply to comment #11) > (From update of attachment 157087 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review > > >>>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64 > >>>> + if not self._patch.committer() and not self._patch.attacher().can_commit: > >>> > >>> Should we have a comment here about why we need a committer here? > >> > >> What kind of comment are we going to add? > > > > Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ... > > Oh, that's merely because I don't trust those patches to be ran on my Mac mini at the moment. > I'm not sure if adding a comment like "rniwa doesn't trust patches submitted by non-committers" is any more helpful than the code itself given that nobody else is going to run this thing > (surely, we can't have two IRC bots named perfalizer). I see, I thought this was some sort of security thing (I think we have this check on the other EWS bots, for example). At any rate, as far as testing goes, can you put in some tests that at least stub out the executive enough to make sure the code executes *at all* (i.e., getting covered, even if we're not testing correctness)?

Created attachment 157334 [details] Added some tests per Dirk's comment

Comment on attachment 157334 [details] Added some tests per Dirk's comment thanks! coverage makes me happier ...

Committed r125123: <http://trac.webkit.org/changeset/125123>