Amphtml: Pick the correct baseline for Percy builds on release branches

[rsimha]

While running visual tests on a release branch, we currently pick the most recent approved build on master as the baseline for diffs. This will result in a false negative if there are approved visual changes in master that were merged after the branch point of the release branch.

See https://percy.io/docs/learn/baseline-picking-logic. We want to go from the "branches" behavior to the "pull requests" behavior, and doing so will require the creation of a pull request prior to running tests on a release branch.

Current behavior:

Intended behavior:

/cc @cramforce @choumx @erwinmombay

[choumx]

👍 for nice graphics. What tool did you use?

[rsimha]

I used the drag and drop images from https://percy.io/docs/learn/baseline-picking-logic tool.

[rsimha]

Update: @choumx and I tried creating PRs on release branches before running visual tests in order to replicate the intended behavior outlined in this issue. However, we're still seeing the old behavior, where the baseline being used is the latest build on master, instead of the most recent build prior to the branch point.

I've tried this with both manual and automatic diff base on Percy. Here is some debugging info:

Pull request from release branch: #11531

With automatic diff base: (black build numbers)
Release branch Percy build: # 5180
Expected baseline Percy build: # 4999
Actual baseline Percy build: # 5179

With manual diff base: (red build numbers)
Release branch Percy build: # 5189
Expected baseline Percy build: # 4902 (approved)
Actual baseline Percy build: # 5114 (approved)

I'll raise this with the Percy folks.

[rsimha]

/cc @cramforce

[rsimha]

/to @timhaines

[rsimha]

Update from meeting with https://github.com/fotinakis: We identified that the automatic baseline picking logic wasn't working in #11531 because the PR was made off of a private branch on rsimha-amp/amphtml, which was forked off of ampproject/amphtml. The github API had no way of knowing the SHA on upstream master at which the branch was forked, and so Percy picked the latest successful build on master as the baseline for the PR build.

[rsimha]

Further investigation from today: @choumx and I took another stab at creating a new PR based on a new release branch off of master, this time directly on ampproject/amphtml (instead of rsimha-amp/amphtml). See #11551.

Results:

1. When the release branch amp-release-10-03-2017-rsimha2 was created, a new Percy build was generated at the branch point. However, the baseline used was the latest build on master, instead of the previous build at that branch point. Therefore, the build contained visual diffs when it should not have done so. See Percy build # 5246.
2. When PR #11551 was created on the release branch, commits were cherry picked, and a PR build was generated, the resulting Percy build correctly used the build on the branch point as its baseline. See Percy build # 5251.

A way to pick the correct baseline in 1. would be for Percy to provide a means of specifying the baseline commit SHA for a build. We can then specify the branch point's SHA as the baseline in 1. This has been conveyed to https://github.com/fotinakis / @timhaines.

I'll follow up on this issue when we have more updates.

[rsimha]

We're seeing more weird baseline picking behavior on ampproject/amphtml due to https://github.com/percy/percy-client/issues/26. I'm following up offline with the Percy folks.

Edit: Weird behavior was due to developers uploading PRs from the master branch on their forks. For example, see #11566. This is now mitigated via #11579.

[rsimha]

I've logged https://github.com/percy/percy-client/issues/27 on the Percy repo to track the addition of a way to specify the baseline commit SHA to be used by a Percy build.

[rudygalfi]

I rolled this forward to the October H2 milestone. Any updates?

[rsimha]

This is currently blocked by the open issues in the percy repo.

[ampprojectbot]

This issue hasn't been updated in awhile. @danielrozenberg Do you have any updates?

[ampprojectbot]

This issue hasn't been updated in awhile. @danielrozenberg Do you have any updates?

[rsimha]

@danielrozenberg Good news! We now have the required support from Percy to fix this. See https://github.com/percy/percy-client/issues/27#issuecomment-424006034, which adds a new PERCY_TARGET_COMMIT environment var.