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

manual-e2e-run #112

Merged
merged 18 commits into from
Oct 30, 2023
Merged

manual-e2e-run #112

merged 18 commits into from
Oct 30, 2023

Conversation

jablonnc
Copy link
Contributor

@jablonnc jablonnc commented Oct 27, 2023

The Issue

As discussed here, running our E2E tests every time a PR is opened and changed is a significant load on our system and can be a burden on the engineer.

This update will change our approach when it comes to how E2E tests are run. As shown in this example, E2E tests are currently baked into our PR workflow. Meaning every time an engineer opens or updates a PR, we are running our E2E tests.

Screenshot 2023-10-27 at 3 22 18 PM

Unlike our other workflow jobs, running E2E tests is costly as they are much slower than our other jobs and also have the potential to consume a lot of runners at once as we allow parallel testing (to help speed up the overall test run). To remedy this, we are taking the E2E test runs out of the PR flows and moving them into flows that can be triggered by leaving a PR comment. In this way, engineers can decide when they are ready to run the tests and we can reduce the burden on the overall system.

The Solution

PR Opened

When a PR is now opened, all the status checks will get updated minus pr / E2E Status which will stay in a state of Waiting for status to be reported as nothing has triggered this status check:

Screenshot 2023-10-27 at 3 52 47 PM

Test Triggered

Upon leaving a comment of /run-e2e-tests on the PR, the status check will be updated to indicate its status is Pending (this means the tests are now getting run):

Screenshot 2023-10-27 at 3 54 51 PM Screenshot 2023-10-27 at 3 54 34 PM

Test Complete

Once the test have passed/failed, the status check will finally enter into a state where the PR can be merged (assuming it was a successful run):

Screenshot 2023-10-27 at 3 48 40 PM

Updates

Application Runtime Flows

As demonstrated here, the flow for application runtime PRs has been simplified and no longer contains E2E test jobs:

Screenshot 2023-10-27 at 3 27 54 PM

As demonstrated here, a new flow was added for running application E2E tests:

Screenshot 2023-10-27 at 3 29 48 PM

These tests will get triggered when a comment containing /run-e2e-tests is left on the PR.

Utility Runtime Flows

As demonstrated here, the flow for utility runtime PRs has been simplified and no longer contains E2E test jobs:

Screenshot 2023-10-27 at 3 35 22 PM

As demonstrated here, a new flow was added for triggering tests from utility runtimes:

Screenshot 2023-10-27 at 3 37 11 PM

These tests will get triggered when a comment containing /run-e2e-tests is left on the PR.

Rollout Strategy

  1. Once this PR is merged to main, it will be tagged at v3. All current consumers are on v2, so there is no risk of breaking changes here.
  2. An ops-platform-job will be created to add a new workflow file for:
  1. Prior to rolling out these changes, an announcement will be made in #dev-frontend of this change and we will make any adjustments necessary once we go live.

Note: I have kept the status check regarding the E2E Status the same, so no modifications should be necessary regarding the current Github rulesets.

Rollback Plan

If necessary we can leverage the [update-github-workflow-version job])https://github.com/JupiterOne/ops-platform-jobs/tree/main/scripts/mass-repo-jobs/update-github-workflow-version) to rollback our workflows and leverage the current v2 release.

@jablonnc jablonnc requested a review from a team as a code owner October 27, 2023 17:19
@jablonnc jablonnc marked this pull request as draft October 27, 2023 18:43
@jablonnc jablonnc marked this pull request as ready for review October 27, 2023 20:00
e2e_containers:
description: "The number of tests that you want Cypress to run in parallel (ex. 1, 2, 3, ...)"
type: string
default: '["1"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value doesn't match the example provided in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of the very first example from web-home, that's the old flow and is also based on the e2e_container input shown here for that repo. Each repo gets to decide how many containers they need for running their tests to optimize their test runs.

In terms of the other examples referenced above, these are from tests being run in specific repos, web-navbar for the Application utility flow and web-query-results for the Utility runtime flow.

The value shown here is for the global workflow and as demonstrated by remarks above, can be overridden by the consumer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, should've been more clear. The description says "ex. 1,2,3", ie: a string, but the default value is a string that is an array of strings ["1"]. So does the user need to provide an array of strings, or just a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay updated the comment to the following:

Screenshot 2023-10-27 at 4 42 07 PM

const name = 'pr / E2E Status';
const url = '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}';
const success = '${{ inputs.job_status }}' === 'success';
const body = `${name}: ${success ? 'succeeded ✅' : 'failed ❌'}\n${url}`;
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice touch, thank you for adding it.

| Name | Required | Description |
| --------------------------- | --------- | ----------------------------------------- |
| `NPM_TOKEN` | True | A J1 npm.com Publish token
| `CYPRESS_RECORD_KEY` | False | The record key associated with the project in Cypress.
Copy link
Member

Choose a reason for hiding this comment

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

Why are these not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. They weren't initially required because the E2E flow was a part of the overall PR flow, and not ever repo runs E2E tests. Now that these are split out they should be required. These have since been updated.

@jablonnc jablonnc merged commit aa08d7e into main Oct 30, 2023
4 checks passed
@jablonnc jablonnc deleted the manual-e2e-run branch October 30, 2023 14:10
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.

5 participants