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

Try running UI tests on any updates to develop #4114

Merged
merged 6 commits into from
Nov 10, 2021

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Oct 13, 2021

Fixes #4101

This PR follows up on the proposal in #4101 to automatically run our full automated test suite on all changes to the develop branch. This is still just a proposal, so the idea of this PR is to try this idea out before we decide on it. If we're happy with it, we can merge this PR to put the proposal into practice.

⚠️ Before merging: If these changes work and are approved, we need to remove the testing branch (this PR's branch) from the config and set post-to-slack to true.

To test

  1. Ensure a new Test iOS on Device - Full (On Updated Develop) is seen below under CI checks
  2. Ensure a new Test Android on Device - Full (On Updated Develop) is seen below under CI checks

Note: The two new workflows will only run on the develop branch once this PR is merged. They only appear on this PR because we're temporarily adding this PR's branch to the list of branches the workflow runs on.

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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@guarani guarani requested a review from mchowning October 14, 2021 21:24
@guarani guarani marked this pull request as ready for review October 14, 2021 21:24
@guarani guarani added the [Status] DO NOT MERGE Do not merge this PR label Oct 14, 2021
@guarani guarani removed the request for review from mchowning October 22, 2021 16:51
@guarani
Copy link
Contributor Author

guarani commented Oct 22, 2021

👋 I'm removing the review request since I noticed how Optional UI Tests is still on hold:

Screen Shot 2021-10-22 at 13 51 57

@guarani guarani requested a review from mchowning November 4, 2021 16:15
@guarani
Copy link
Contributor Author

guarani commented Nov 4, 2021

👋 Hi @mchowning, this is ready for review again.

There's some repetition in the CircleCI config. For example, ios-device-checks is called three different times and the only difference between them is the name given to the job and the branch the job is triggered on. While it might be worthwhile finding a more concise syntax, CircleCI config will be replaced with Buildkite config in the future, so any optimizations might no longer be necessary.

@guarani guarani added the Tooling label Nov 4, 2021
@guarani
Copy link
Contributor Author

guarani commented Nov 8, 2021

Thanks for the review, @mchowning! I've addressed the feedback and this should be ready to review again.

@guarani guarani requested a review from mchowning November 8, 2021 21:21
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @guarani !

There's some repetition in the CircleCI config. For example, ios-device-checks is called three different times and the only difference between them is the name given to the job and the branch the job is triggered on. While it might be worthwhile finding a more concise syntax, CircleCI config will be replaced with Buildkite config in the future, so any optimizations might no longer be necessary.

For merging, would it be cleaner to just remove the two new jobs you created and instead add the develop branch to the "release" jobs and give those jobs a more generic name?

We probably should go ahead and switch from develop to trunk as well since the main apps are making that switch, but that's not an issue that needs to be addressed as part of this PR.

@guarani
Copy link
Contributor Author

guarani commented Nov 10, 2021

For merging, would it be cleaner to just remove the two new jobs you created and instead add the develop branch to the "release" jobs and give those jobs a more generic name?

That's a good point. I'd considered it in a similar PR (for running these tests on release PRs) but forgot to mention the discussion here. In short, my position was that it would be valuable to have separate names for each. While the Slack notification doesn't show these custom names, clicking on the notification opens CircleCI which displays the job name prominently, which I think is useful. Otherwise you'd need to look at the branch name in the CircleCI job if you wanted to know what type of test was run. I have no strong preference here though, but figured this was the safer option for now. Let me know if you think we should change this in a follow-up PR.

Thanks for the review!

@guarani guarani merged commit f051d30 into develop Nov 10, 2021
@guarani guarani deleted the try/running-ui-tests-on-updated-develop branch November 10, 2021 13:47
@guarani
Copy link
Contributor Author

guarani commented Nov 10, 2021

⚠️ Before merging: If these changes work and are approved, we need to remove the testing branch (this PR's branch) from the config and set post-to-slack to true.

I didn't do this before merging, so expect a follow-up PR 🙈
Update: Clean-up will be done in #4224, will request a review on it once its checks have passed.

guarani added a commit that referenced this pull request Nov 10, 2021
I merged #4114 without first removing the the test code, so this change does that clean-up.
@guarani
Copy link
Contributor Author

guarani commented Nov 10, 2021

👋 @mchowning, the clean-up PR and I added you as a reviewer if you don't mind.
Update: Clean up PR merged.

@fluiddot fluiddot added this to the 1.66.0 (18.7) milestone Nov 11, 2021
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.

Run our full automation test suite on all new changes to the develop branch
3 participants