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

e2e: updating PR pipeline #19478

Merged
merged 1 commit into from
Apr 3, 2024
Merged

e2e: updating PR pipeline #19478

merged 1 commit into from
Apr 3, 2024

Conversation

churik
Copy link
Member

@churik churik commented Apr 2, 2024

  • added process for skip e2e
  • added requirements for QA review that were discussed here
  • other minor improvements

@status-im-auto
Copy link
Member

status-im-auto commented Apr 2, 2024

Jenkins Builds

Click to see older builds (4)
Commit #️⃣ Finished (UTC) Duration Platform Result
5980b03 #1 2024-04-02 15:25:28 ~4 min tests 📄log
✔️ 5980b03 #1 2024-04-02 15:34:42 ~13 min ios 📱ipa 📲
✔️ 150419d #2 2024-04-02 15:41:22 ~5 min android-e2e 🤖apk 📲
✔️ 150419d #2 2024-04-02 15:43:50 ~8 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ edf9e2c #4 2024-04-03 15:33:03 ~4 min tests 📄log
✔️ edf9e2c #4 2024-04-03 15:35:41 ~7 min android-e2e 🤖apk 📲
✔️ edf9e2c #4 2024-04-03 15:35:55 ~7 min android 🤖apk 📲
✔️ edf9e2c #4 2024-04-03 15:38:26 ~10 min ios 📱ipa 📲
✔️ 79aada9 #5 2024-04-03 15:48:31 ~4 min tests 📄log
✔️ 79aada9 #5 2024-04-03 15:52:06 ~8 min android-e2e 🤖apk 📲
✔️ 79aada9 #5 2024-04-03 15:52:12 ~8 min android 🤖apk 📲
✔️ 79aada9 #5 2024-04-03 15:54:56 ~11 min ios 📱ipa 📲


### Adding `skip-manual-qa`

- Please ask another team member before adding the `skip-manual-qa` label (or here in chat) so that there's a second opinion.
Copy link
Contributor

Choose a reason for hiding this comment

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

(or here in chat)

looks like it should be (or on Discord/Status community/DMs) or smth like that instead

Information on how to analyze tests can be found [here](https://github.com/status-im/status-mobile/blob/develop/doc/how-to-launch-e2e.md).
Tests might be flaky, as they depend on infrastructure - SauceLabs and Waku.

If there are `Failed tests` and you are not sure about the reason, you can always ping the mobile QAs for help (preferably in PRs).
Copy link
Contributor

@qoqobolo qoqobolo Apr 2, 2024

Choose a reason for hiding this comment

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

I think you can also mention here that devs should use our group tag @status-im/mobile-qa in this case

4. Has the label: `request-manual-qa`
1. Reviewed and has at least 1 approval.
2. Rebased to the `develop` branch (both `status-mobile` and `status-go` if needed, depending on what part has changes).
- **NOTE:** QAs can use the `Update with rebase` button from GitHub in case if:
Copy link
Member

Choose a reason for hiding this comment

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

Minor grammatical thing

Suggested change
- **NOTE:** QAs can use the `Update with rebase` button from GitHub in case if:
- **NOTE:** QAs can use the `Update with rebase` button from GitHub if:

or

Suggested change
- **NOTE:** QAs can use the `Update with rebase` button from GitHub in case if:
- **NOTE:** QAs can use the `Update with rebase` button from GitHub in case of:

### Adding `skip-manual-qa`

- Please ask another team member before adding the `skip-manual-qa` label (or here in chat) so that there's a second opinion.
- The PR should have a proper reasoning why manual QA is skipped.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The PR should have a proper reasoning why manual QA is skipped.
- The PR MUST have a proper reasoning why manual QA is skipped.


- Please ask another team member before adding the `skip-manual-qa` label (or here in chat) so that there's a second opinion.
- The PR should have a proper reasoning why manual QA is skipped.
- The PR should include the steps of testing that has been done by the developer prior to moving it forward.
Copy link
Member

@Samyoul Samyoul Apr 2, 2024

Choose a reason for hiding this comment

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

I don't know if this one needs to be a MUST, but I've suggested it

Suggested change
- The PR should include the steps of testing that has been done by the developer prior to moving it forward.
- The PR MUST include the steps of testing that has been done by the developer prior to moving it forward.

**From the perspective of a developer it means that once work on PR is finished:**

1. It should be rebased to the latest `develop`. If there are conflicts - they should be resolved if possible.
2. If the PR was in the `Contributor` column - it should be moved to `Review` column.
3. Wait for the review.
4. Make sure that after review and before requesting manual QA your PR is rebased to current develop.
5. Once the PR has been approved by reviewer(s) - label `request-manual-qa` should be applied to the PR
6. Move PR to the E2E column when it is ready for testing (**mandatory for all PRs**). That will also trigger e2e tests run. QAs are monitoring PRs from E2E column and take it into test.
6. **MUST:** Move PR to the E2E column when it is ready for testing (**mandatory for all PRs**). That will also trigger e2e tests run. QAs are monitoring PRs from E2E column and take it into test.
Copy link
Member

Choose a reason for hiding this comment

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

This makes more grammatical sense to me, but I also may be misunderstanding.

If the full sentence is

From the perspective of a developer it means that once work on PR is finished:

Then the below sentence makes more sense to me.

Suggested change
6. **MUST:** Move PR to the E2E column when it is ready for testing (**mandatory for all PRs**). That will also trigger e2e tests run. QAs are monitoring PRs from E2E column and take it into test.
6. The PR **MUST** be moved to the E2E column when it is ready for testing (**mandatory for all PRs**). That will also trigger e2e tests run. QAs are monitoring PRs from E2E column and take it into test.

@@ -68,7 +98,7 @@ There are three possible scenarios when the design review is completed:
---

#### Why my PR is in `Contributor` column?
PR can be moved to this column by the ```status-github-bot``` or by QA engineer with label `Tested-issues`.
PR can be moved to this column by the ```status-github-bot``` or by QA engineer with label `Tested-issues` or if one of the requirement for manual QA was not met.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PR can be moved to this column by the ```status-github-bot``` or by QA engineer with label `Tested-issues` or if one of the requirement for manual QA was not met.
PR can be moved to this column by the ```status-github-bot``` or by QA engineer with label `Tested-issues` or if one of the requirements for manual QA was not met.

Comment on lines 63 to 66
1. PR has a description and the area affected.
2. PR has automation results from the previous step.
3. PR includes a demo, test steps, and possible regression
4. PR has out of scope (if appropriate)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. PR has a description and the area affected.
2. PR has automation results from the previous step.
3. PR includes a demo, test steps, and possible regression
4. PR has out of scope (if appropriate)
1. PR has a description of the change and details the area of the application affected by the change.
2. PR has automation results from the previous E2E test run step.
3. PR includes a demo, test steps, and any possible regression(s)
4. If appropriate, PR details what elements of a feature are out of scope.

Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

These changes look good, I've added some suggestions to make the language more clear but I have no objections at all to the policy.

@churik churik force-pushed the docs/update-pipeline branch 3 times, most recently from edf9e2c to 79aada9 Compare April 3, 2024 15:43
@churik churik merged commit 354cda3 into develop Apr 3, 2024
6 checks passed
@churik churik deleted the docs/update-pipeline branch April 3, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants