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

Run full e2e test suite on dependabot submodule update PRs #2789

Merged
merged 6 commits into from
Nov 9, 2020

Conversation

ceyhun
Copy link
Contributor

@ceyhun ceyhun commented Nov 9, 2020

Run full e2e test suite on dependabot submodule update PRs and post failures to slack

To test: Check if Test iOS on Device - Submodule Update and Test Android on Device - Submodule Update are run without any approval.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@hypest
Copy link
Contributor

hypest commented Nov 9, 2020

Nice tweak, thanks Ceyhun! Makes total sense to have the full set of tests run automatically on these hash-update PRs!

While we're at it I'm thinking: Dependabot doesn't offer PR grouping yet, to have 1 single PR with the Gutenberg and the Jetpack submodule for example, leading to too many tests running automatically like that, let alone not being super sure that the two updates are successfully stacked unless we manually ask Dependabot to rebase after we merge one of the two. So, to save time and tests CI occupancy, what do you think about setting https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/configuration-options-for-dependency-updates#open-pull-requests-limit to 1 so there will be only 1 outstanding PR? Idea is that we'll let either the Gutenberg or the Jetpack hash update PR finish and get merged first and the Dependabot will open the other.

@ceyhun
Copy link
Contributor Author

ceyhun commented Nov 9, 2020

Thanks for the idea @hypest. I have 2 things I'm not sure about this:

  1. What if it doesn't open another PR until next day, because the interval is daily?
  2. What if it opens another gutenberg PR instead of a jetpack one?

We have to try and see I guess?

@ceyhun ceyhun requested a review from hypest November 9, 2020 13:02
@ceyhun ceyhun marked this pull request as ready for review November 9, 2020 13:02
@hypest
Copy link
Contributor

hypest commented Nov 9, 2020

Yeah, I thought of case No2 (which should be kinda OK to favor Gutenberg updates more often than Jetpack's) but case No1 might still favor Gutenberg's hash and that'd be too much.

Let's stick with the current limit (5 PRs by default) and we'll see. The time these PRs will open (5am UTC) are perhaps early enough that it won't be rush hour anyway.

@hypest
Copy link
Contributor

hypest commented Nov 9, 2020

With the full testsuite now running automatically on the hash-update Dependabot PRs, I think we can also experiment with auto-merging. I've enabled the option in our account and if you agree Ceyhun, we can add the support via this PR. What we need to do is update the dependabot config with something like this:

version: 2
updates:
  - package-ecosystem: "gitsubmodule" # See documentation for possible values
    directory: "/" # Location of package manifests
    schedule:
      interval: "daily"
+   automerged_updates:
+     - match:
+         dependency_name: "gutenberg"
+     - match:
+         dependency_name: "jetpack"

What do you think?

@ceyhun
Copy link
Contributor Author

ceyhun commented Nov 9, 2020

Sounds good to me, will update the config 👍

@hypest
Copy link
Contributor

hypest commented Nov 9, 2020

Ouch, I should have looked deeper. It looks like auto-marging is not supported in the GitHub-native Dependabot :( dependabot/dependabot-core#1973 (comment)

@ceyhun
Copy link
Contributor Author

ceyhun commented Nov 9, 2020

Indeed :( got an error in the checks saying Your .github/dependabot.yml contained invalid details. Will revert this change.

@hypest
Copy link
Contributor

hypest commented Nov 9, 2020

Yeah, sorry for the time lost there Ceyhun :(

On a different note, I wonder if the peril prompt to run the full testsuite might be confusing to people. I didn't see it popup in the first full run of the tests so I thought it wouldn't trigger, but obviously it does. 🤔

@ceyhun
Copy link
Contributor Author

ceyhun commented Nov 9, 2020

I think it was checking for the job named Optional UI Tests: https://github.com/Automattic/peril-settings/blob/685783686a07f5a09ee8d65acbf96335f7b0b146/org/pr/gb-mobile-ui-tests.ts#L7
Ignored it for submodule update branches.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM and it it will be very useful to have in place, thanks Ceyhun. If the Peril prompt persists we'll address it in a separate PR 👍

@hypest hypest merged commit 1b6bf21 into develop Nov 9, 2020
@hypest hypest deleted the dependabot/submodules/try-running-full-tests branch November 9, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants