Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix percy target branch for PRs #43160

Merged
merged 11 commits into from
Aug 14, 2019
Merged

Conversation

liza-mae
Copy link
Contributor

@liza-mae liza-mae commented Aug 12, 2019

After we enabled visual testing for each PR in #42989 I noticed the PR branch is used as the baseline which would cause each PR to have an unreviewed Percy check (failed Github check). I think the baseline for PRs should be master.

Relates to #33817

@liza-mae liza-mae requested a review from a team as a code owner August 12, 2019 22:51
@liza-mae liza-mae self-assigned this Aug 12, 2019
@@ -25,5 +25,8 @@ const shortCommit = commit.slice(0, 8);

const isPr = process.env.JOB_NAME.includes('elastic+kibana+pull-request');

const { stdout: branch } = execa.sync('git', ['rev-parse', '--abbrev-ref', 'HEAD']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this not to be what is tracked in the package.json? https://github.com/elastic/kibana/blob/master/package.json#L15

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep it separate so that feature branches can deviate from the base snapshots.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@liza-mae
Copy link
Contributor Author

So the branch baseline is origin/master and I was originally pointing to master using process.env.ghprbTargetBranch, but changing to PR_TARGET_BRANCH it returns the same.
Still trying to figure out why Percy is using origin/master.

The CI failure is related to Firefox smoke test and the Percy failure is on the lower left Kibana icon slight shift, same characteristics as other failures I have seen, hoping it is one root cause and we can fix, looking.

@liza-mae
Copy link
Contributor Author

So origin/master is being set automatically since Jenkins is fetching remote origin. Although according to Percy support it would be best to have this set to master and we would need to set env variable PERCY_BRANCH also.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Would you please send out an email once this is merged explaining what percy is, that regular contributors should check their email for an invite, and how they can request access if they don't have one.

@liza-mae
Copy link
Contributor Author

Sounds good @spalger -- will do :) thanks !

@liza-mae liza-mae merged commit fcba9a6 into elastic:master Aug 14, 2019
@liza-mae liza-mae deleted the liza-fix/percy-branch branch August 14, 2019 16:44
liza-mae added a commit that referenced this pull request Aug 14, 2019
* Fix percy target branch for PRs

* Add debug logging

* Try diff branch name

* Use PR_TARGET_BRANCH instead

* renable visReg jobs

* parse target branch from `branch_specifier`

* rename debugging print out

* set `PERCY_BRANCH` and `PERCY_TARGET_BRANCH`

* log PERCY_TARGET_BRANCH

* cleanup error messages
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 15, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (50 commits)
  [Uptime] update monitor list configs for mobile view (elastic#43218)
  [APM] Local UI filters (elastic#41588)
  [Code] Upgrade ctags langserver (elastic#43252)
  [Code] show multiple definition results in panel (elastic#43249)
  Adds Metric Type to full screen launch tracking (elastic#42692)
  [Canvas] Convert Autocomplete to Typescript (elastic#42502)
  [telemetry] add spacesEnabled config back to xpack_main (elastic#43312)
  [ML] Adds DF Transform Analytics list to Kibana management (elastic#43151)
  Add TLS client authentication support. (elastic#43090)
  [csp] Telemetry for csp configuration (elastic#43223)
  [SIEM] Run Cypress Tests Against Elastic Cloud & Cypress Command Line / Reporting (elastic#42804)
  docs: add tip on agent config in a dt (elastic#43301)
  [ML] Adding bucket span estimator to new wizards (elastic#43288)
  disable flaky tests (elastic#43017)
  Fix percy target branch for PRs (elastic#43160)
  [ML] Adding post create job options (elastic#43205)
  Restore discover histogram selection triggering fetch (elastic#43097)
  Per panel time range (elastic#43153)
  [Infra UI] Add APM to Metadata Endpoint (elastic#42197)
  Sentence case copy changes (elastic#43215)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants