Come up with strategy for running Likadan in a CI environment

[trotzig]

One of the main reasons for creating Likadan was that I wanted to run perceptual diffs in a CI environment. So far, I've mainly focused on the tool itself, and not the CI process. But I think it's time Likadan is enhanced even further so that it could be used alongside unit/feature/integration tests, linters, and other tools that aim at automatically making sure that the code is of high quality and prevent bugs.

The straight-forward way would be to run Likadan on each commit, and fail if a diff is found. It's not that simple however. I've noticed that after running Likadan manually once every day on the Brigade code base for the last month. I did this because I wanted to get more experience with the type of diffs that are normally caught. The Likadan suite for Brigade consists of 64 examples. Everything from smaller components (buttons, tooltips) to larger components (dialogs, cards) is tested. Here are a few things I've learned from running it:

• Few diffs are actual regressions in style
• Many diffs are intentional, and caused by iterating on the design of a component
• A number of diffs are somewhat unintentional, but harmless (changing a global color), refactoring some css, etc.

To make Likadan fully automated, we need to separate the real regression diffs from other diffs. I'm not sure how to do that. There's always a bit of human activity needed to figure out which diffs are bad and which are good. Lately I've been thinking about how we can aid manual diffing with automated diffing. What if Likadan posted the diffs as a comment to the commit? Then it would be easy for a code reviewer (someone else or the author) to make the call. If you like Brigade use Gerrit for code review, we could post a comment with a link to the diffs (I'm not sure you can inline images in Gerrit). If you use the Github PR model, we could post a comment with inline diff images to the pull request.

The basic CI script for likadan would

1. Check out the code before the commit
2. Run likadan to generate baseline snapshots
3. Check out the new commit
4. Run likadan again
5. Report back result

Another option I've considered is tagging a commit that you know will result in a diff with something that Likadan can use to auto-approve diffs. Let's pretend the commit message looks like this:

Make buttons wider

We want buttons to be easier to target. The previous width 
wasn't enough for people with small thumbs.

[likadan changes "button"]
[likadan changes "button with icon"]
[likadan changes "button group"]

Then when the Likadan suite is run, the tagged diffs will be ignored. I'm not a fan of this solution right now though. I don't want to pollute the git history with those tags, plus I think it's better to make the call at review time.

What do you think? Can you come up with an even better alternative, or perhaps refine what I'm proposing?

[flarnie]

I like the idea of having perceptual diffs in a CI environment, and I think the process you described (Check out the code before the commit, Run likadan to generate baseline snapshots, Check out the new commit, Run likadan again, Report back result) makes sense.

With the Brigade codebase (atomic commits) in particular, I'm not sure it would make sense to have CI run Likadan on every commit.

• How long does Likadan take to run?
• How much time would it take to review the 'diff' image(s) on that commit before merging?

If the above times are minimal or could be minimized, then it's fine to run it on every commit, but otherwise it would be better to set it up to run every 15 minutes or something, and email/slack message us if there are diffs and which components they are on.

I also remember the set-up of a daily email from Diffux with diffs, and that would be a good enough way to use Likadan. It would be equivalent to what you have done manually, and we have still caught bugs as a result of that. :)

[janpaul123]

I like your proposal @trotzig. I think running on every commit is fine with the current performance, and most useful. In any case it would not be blocking our normal flow. I also like the idea of automating the summary email, which you have done manually for now.

To prevent having to make Github/Gerrit integrations right away, what about just emailing the author(s) of a commit? That will work for any git repo.

[lencioni]

The straight-forward way would be to run Likadan on each commit, and fail if a diff is found.

I'm not sure it makes sense to fail. Is there a "warning" option that we could use? If not, fail seems fine since we can override.

What if Likadan posted the diffs as a comment to the commit?

This sounds good to me.

Another option I've considered is tagging a commit that you know will result in a diff with something that Likadan can use to auto-approve diffs.

I don't think we should do this because we would likely end up saying "this commit changes this component visually" for changes that have unexpected visual consequences in those components. Plus, the overhead of adding this to your commit messages likely is higher than the proposed alternative.

I'm not sure it would make sense to have CI run Likadan on every commit.

I very much prefer running it on every commit. This gives the individual feedback that they need at precisely the time where it can be most useful. The developer can then decide if they need to fix it in that commit, follow up, or ignore. If it is run every 15min or so, who will be notified and who will be responsible for dealing with diffs?

I think that most commits will result in no diffs, so this should not be much of a burden.

[trotzig]

@flarnie, @lencioni, @janpaul123, thanks for the input!

I'm pretty convinced that we need to run this on every commit. The burden of manually tracking down what caused a diff is time-consuming, and it would be much better with quick feedback.

I like @janpaul123 's suggestion about sending out an email with the results. That's a good first start at least, before we start writing integrations with Gerrit, Github and other tools.

About performance: The likadan suite itself runs is pretty quick (runs in under a minute I think). Compiling the webpack bundles however, is where most time is spent.

[lencioni]

We've recently had some styling regressions at Brigade. Running likadan manually later really helped, but I think that CI integration would have prevented it entirely. @trotzig what do you think would be involved in landing this at this point? @sds might have some ideas for how we can hook directly into Gerrit.

[sds]

I think building a lightweight web service will allow you to solve this effectively. Unfortunately it will be a fair bit of work.

The "Cool" Solution

In order to make this play well with Gerrit where "+1 Verified" is an important part of the flow (i.e. allows you to block the merging of a commit until it's explicitly +1ed), you're going to want to have a point in the process where a human approval of the diff results in a +1 sent to Gerrit (and only if a commit requires a Likadan run—or are we assuming all commits will?).

By running Likadan as a web service, a Jenkins job can POST to Likadan with the information necessary to compare the current commit to the previous. Likadan runs asynchronously and then posts a comment on the Gerrit change with a link to the results of the run if verification by a human is required; otherwise it just posts a +1 Verified.

The link will take the developer to a page on the Likadan service itself showing the perceptual diff, the developer will approve/deny the changes, and then Likadan will +1/-1 the change appropriately in Gerrit.

In order to have Jenkins not +1 Verify when a Likadan build is triggered, we'll need a custom script that Jenkins executes on Gerrit which reads the build output, checks if Likadan was triggered for the commit, and then doesn't +1 if a Likadan build is in process (this is to prevent the Likadan build from hogging Jenkins slaves waiting on the Likadan server), otherwise it +1s if tests from all the other triggered builds passed.

OR....

Obviously, this is a fair amount of complexity that is mostly in service of supporting the +1 Verified flow. If we are willing to keep the current behavior and have Likadan not involved in the +1 Verify step, we can have it just post a comment to a change with a link to the diff, and require humans resist the urge to click the submit button until they visit that link and confirm the diff. Probably easier (and the preferred approach from an operations perspective).

[trotzig]

I'm perfectly fine starting with the simpler solution to begin with. On Jul 18, 2015 12:30 AM, "Shane da Silva" notifications@github.com wrote:

I think building a lightweight web service will allow you to solve this effectively. Unfortunately it will be a fair bit of work. The Cool Solution

In order to make this play well with Gerrit where "+1 Verified" is an important part of the flow (i.e. allows you to block the merging of a commit until it's explicitly +1ed), you're going to want to have a point in the process where a human approval of the diff results in a +1 sent to Gerrit (and only if a commit requires a Likadan run—or are we assuming all commits will?).

By running Likadan as a web service, a Jenkins job can POST to Likadan with the information necessary to compare the current commit to the previous. Likadan runs asynchronously and then posts a comment on the Gerrit change with a link to the results of the run if verification by a human is required; otherwise it just posts a +1 Verified.

The link will take the developer to a page on the Likadan service itself showing the perceptual diff, the developer will approve/deny the changes, and then Likadan will +1/-1 the change appropriately in Gerrit.

In order to have Jenkins not +1 Verify when a Likadan build is triggered, we'll need a custom script that Jenkins executes on Gerrit which reads the build output, checks if Likadan was triggered for the commit, and then doesn't +1 if a Likadan build is in process (this is to prevent the Likadan build from hogging Jenkins slaves waiting on the Likadan server), otherwise it +1s if tests from all the other triggered builds passed. OR....

Obviously, this is a fair amount of complexity that is mostly in service of supporting the +1 Verified flow. If we are willing to keep the current behavior and have Likadan not involved in the +1 Verify step, we can have it just post a comment to a change with a link to the diff, and require humans resist the urge to click the submit button until they visit that link and confirm the diff. Probably easier (and the preferred approach from an operations perspective).

— Reply to this email directly or view it on GitHub https://github.com/trotzig/likadan/issues/4#issuecomment-122436126.

[lencioni]

Perhaps add a section to the readme about how to set it up this way, and then this issue can probably be closed out, eh?

[trotzig]

Sounds like a plan. I'll get back to this issue once I'm back in civilization next week.