search

ember-learn / guides-source

This repository contains the Ember.js Guides
https://guides.emberjs.com/
MIT License
158 stars 498 forks source link

Adjust Percy build settings #723

Closed jenweber closed 5 years ago

jenweber commented 5 years ago

Whenever we open a PR, there are 2 builds that run from Travis, and only one of them has the branch info that Percy needs in order to know which diffs to use. We need to adjust our test suite to only run in a way that sends the branch info that Percy needs. Maybe we can just disable the 2nd build?

My message to Percy:

Hello hello! I was hoping you could help me figure out why my Percy branch management is a bit confused. Looking at the docs here (https://docs.percy.io/docs/environment-variables) it would seem that Percy should auto-detect the base from a PR, but I'm not seeing that happen. I have two branches that PRs go into, master and octane. However the visual diffs all seem to be running against master, not picking up the target branch. Any ideas? Here's an example: https://percy.io/Ember/guides-app/builds/1699681 and the PR https://github.com/ember-learn/guides-source/pull/722. Percy targets master, the PR targets octane Thank you for your help!

Robdel12 commented 5 years ago

Hey Jen!

I followed up with you in intercom but wanted to make sure I commented here too.

It looks like we did properly select the target branch for this build: https://percy.io/Ember/guides-app/builds/1699680 But another Percy build was ran right after, which is what was used as the status check on the PR. That one is missing some information we need to select the proper target.

What's going on here is TravisCI is running two different builds. One has a variable we need to be able to figure out the target branch (TRAVIS_PULL_REQUEST: https://github.com/percy/percy-js/blob/master/src/environment.js#L230) and the other doesn't. The TravisCI "push" build finished after the PR one, so that's the one that showed up on Percy status check. Here's a screenshot I've annotated of the two status checks. The Travis "push" one will not have the right information for us to select a base build.

travis-ci-builds

Not quite sure what the fix would be other than on PRs only having the PR build run from Travis. I'm not quite sure what tradeoffs that might have, though. It would be nice if Travis recognized that those pushes were on a PR and included TRAVIS_PULL_REQUEST, which is essential for us picking the right baseline based off of the PRs target branch.

jenweber commented 5 years ago

Hey Robert, thanks for the note here & through Percy! I'm sure there's something that could be done here. I'm not even sure what the purpose of running two builds is, for this repo.

jenweber commented 5 years ago

PS Percy is awesome

mansona commented 5 years ago

@jenweber so the reason we have this is because for PRs to the Octane branch we wanted to have CI running, which meant that we needed to turn off the branches: only master thing in travis.yml

The issue that we have is that we can't just add branches: master & octane here because github does a special thing for PRs to master 😞 and I don't know how to make travis only build once for PRs to master and PRs to octane 😭

let me know if I need to explain this more 👍

jenweber commented 5 years ago

Closed, thanks all for the input. We will live with it as-is.

Robdel12 commented 5 years ago

Oh! You could probably toggle the PERCY_ENABLE env var for non-pr builds? 🤔