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

[CI] allow for manual triggered cypress test jobs to accept inputs #5134

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Sep 27, 2023

Description

Utilizing the GitHub workflow_dispatch, we can trigger a job with a specific inputs.

By default, we run from this repo:
https://github.com/opensearch-project/opensearch-dashboards-functional-test

This workflow uses the target branch of the PR to pull down from the FTRepo and run the tests from that specific branch.

For example, PRs against main will pull down main from the FTRepo.

The problem occurs when new functionality is opened or a bug is fixed and, per industry standard, we want to see tests added to ensure functionality, stability, and raise the bar.

However, the cypress tests PR into the FTRepo depends on the new code to be merged for the CI within the FTRepo to work. So we have a stalemate that usually slows down PR review time.

Here, a manual run can be triggered by a maintainer. It will utilize the source branch from the PR if provided.

Issues Related

#4019

Screenshot

Unfortunately, GitHub only enables the getting of ref from the manual run. To do this, a maintainer will have to place inputs from:

Actions > Run cypress tests

Which you can enter inputs and trigger the job:
Screenshot 2023-09-27 at 5 02 23 AM

Results will show up like this if there is associated PR to the triggered run:
Screenshot 2023-09-27 at 4 53 44 AM

As you can see it ran from my own fork but i was able to run a different branch from my own fork of the functional test repo. This is like a manually verification still, I have not looked into updating the status of a PR but I believe we can set to pass still.

Testing the changes

I was playing around with this PR by changing my default.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@kavilla kavilla changed the title [CI] allow for manual triggered jobs to accept inputs [CI] allow for manual triggered cypress test jobs to accept inputs Sep 27, 2023
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #5134 (439537c) into main (46d7c2d) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5134   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files        3403     3403           
  Lines       65026    65026           
  Branches    10401    10401           
=======================================
+ Hits        43243    43245    +2     
+ Misses      19214    19213    -1     
+ Partials     2569     2568    -1     
Flag Coverage Δ
Linux_1 34.82% <ø> (ø)
Linux_2 55.31% <ø> (ø)
Linux_3 44.61% <ø> (+<0.01%) ⬆️
Linux_4 34.89% <ø> (ø)
Windows_1 34.83% <ø> (ø)
Windows_2 55.27% <ø> (ø)
Windows_3 44.61% <ø> (ø)
Windows_4 34.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Utilizing the GitHub `workflow_dispatch`, we can trigger
a job with a specific inputs.

By default, we run from this repo:
https://github.com/opensearch-project/opensearch-dashboards-functional-test

This workflow uses the target branch of the PR to pull down
from the FTRepo and run the tests from that specific branch.

For example, PRs against `main` will pull down `main` from
the FTRepo.

The problem occurs when new functionality is opened or a bug is
fixed and, per industry standard, we want to see tests added
to ensure functionality, stability, and raise the bar.

However, the cypress tests PR into the FTRepo depends on the new code
to be merged for the CI within the FTRepo to work. So we have a
stalemate that usually slows down PR review time.

Here, a manual run can be triggered by a maintainer. It will utilize
the source branch from the PR if provided.

Related to:
opensearch-project#4019

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla force-pushed the kavilla/ci/cypress_input branch from 17334b4 to 439537c Compare September 27, 2023 21:24
SOURCE_REPO: ${{ github.repository }}
SOURCE_BRANCH: ${{ github.base_ref }}
TEST_REPO: ${{ inputs.test_repo != '' && inputs.test_repo || 'opensearch-project/opensearch-dashboards-functional-test' }}
TEST_BRANCH: "${{ inputs.test_branch != '' && inputs.test_branch || github.base_ref }}"
Copy link
Collaborator

@AMoo-Miki AMoo-Miki Sep 27, 2023

Choose a reason for hiding this comment

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

If a PR is raised against a feature branch, github.base_ref would point to that branch; what if that branch doesn't exist on the FT repo?

PS, i see that we currently don't handle that situation anyway!

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. that's happened a couple of times in the pass.

i end up just creating the branch in the FTRepo. Part of the release improvements and artifacts we want to have fallback solutions.

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

cool

@joshuarrrr
Copy link
Member

@kavilla Not to scope creep this, but some documentation about when and how to run this would be useful to be able to reference without coming back to this PR. I know we've talked about a PR review markdown doc, but it could go in one of our existing docs, too.

@kavilla
Copy link
Member Author

kavilla commented Sep 27, 2023

@kavilla Not to scope creep this, but some documentation about when and how to run this would be useful to be able to reference without coming back to this PR. I know we've talked about a PR review markdown doc, but it could go in one of our existing docs, too.

Yeah I was thinking too because the inputs vs defaults I can see being misunderstood. I was originally thinking adding a statement to the CONTRIBUTING doc and will create a subsequent issue for a PR.md

@kavilla
Copy link
Member Author

kavilla commented Sep 27, 2023

Just realized the cypress tests are failing for most PRs now.

Will look I know we addressing some with the Discover work. This will assist with seeing if PRs in the FTRepo so I will merge as is.

I have created a follow-up issue for docs: #5139

@kavilla kavilla merged commit 34a8594 into opensearch-project:main Sep 27, 2023
53 of 55 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 27, 2023
Utilizing the GitHub `workflow_dispatch`, we can trigger
a job with a specific inputs.

By default, we run from this repo:
https://github.com/opensearch-project/opensearch-dashboards-functional-test

This workflow uses the target branch of the PR to pull down
from the FTRepo and run the tests from that specific branch.

For example, PRs against `main` will pull down `main` from
the FTRepo.

The problem occurs when new functionality is opened or a bug is
fixed and, per industry standard, we want to see tests added
to ensure functionality, stability, and raise the bar.

However, the cypress tests PR into the FTRepo depends on the new code
to be merged for the CI within the FTRepo to work. So we have a
stalemate that usually slows down PR review time.

Here, a manual run can be triggered by a maintainer. It will utilize
the source branch from the PR if provided.

Related to:
#4019

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit 34a8594)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@github-actions
Copy link
Contributor

[MANUAL CYPRESS TEST RUN RESULTS]

❌ Cypress test run failed!

Inputs:

Source repo: opensearch-project/OpenSearch-Dashboards
Source branch: Test repo: `abbyhu2000/opensearch-dashboards-functional-test` Test branch:discover_app``

Link to results:

https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/6332816838

kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Sep 28, 2023
Originally:
opensearch-project#5134

I believed it to be a good idea to ensure that triggering the CI
should always run a batch of tests. But I realized that some tests
can be added and just be bad tests until updated. Or if we want
to verify a flaky test in the CI.

So instead of only making it additional tests, the spec input will
complete replace the default tests (unless you append to the default).

Empty will run the default spec still. This will append on to the
PR comment with the spec that was run. So at least maintainers can
see it passed for a set of tests but it did not run the ones
we usually run.

Also adding some formatting for empty values for the PR comment.

Issue related:
opensearch-project#4019

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit that referenced this pull request Sep 29, 2023
Originally:
#5134

I believed it to be a good idea to ensure that triggering the CI
should always run a batch of tests. But I realized that some tests
can be added and just be bad tests until updated. Or if we want
to verify a flaky test in the CI.

So instead of only making it additional tests, the spec input will
complete replace the default tests (unless you append to the default).

Empty will run the default spec still. This will append on to the
PR comment with the spec that was run. So at least maintainers can
see it passed for a set of tests but it did not run the ones
we usually run.

Also adding some formatting for empty values for the PR comment.

Issue related:
#4019

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 2, 2023
Originally:
#5134

I believed it to be a good idea to ensure that triggering the CI
should always run a batch of tests. But I realized that some tests
can be added and just be bad tests until updated. Or if we want
to verify a flaky test in the CI.

So instead of only making it additional tests, the spec input will
complete replace the default tests (unless you append to the default).

Empty will run the default spec still. This will append on to the
PR comment with the spec that was run. So at least maintainers can
see it passed for a set of tests but it did not run the ones
we usually run.

Also adding some formatting for empty values for the PR comment.

Issue related:
#4019

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit 6154e2d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla pushed a commit that referenced this pull request Oct 2, 2023
Originally:
#5134

I believed it to be a good idea to ensure that triggering the CI
should always run a batch of tests. But I realized that some tests
can be added and just be bad tests until updated. Or if we want
to verify a flaky test in the CI.

So instead of only making it additional tests, the spec input will
complete replace the default tests (unless you append to the default).

Empty will run the default spec still. This will append on to the
PR comment with the spec that was run. So at least maintainers can
see it passed for a set of tests but it did not run the ones
we usually run.

Also adding some formatting for empty values for the PR comment.

Issue related:
#4019


(cherry picked from commit 6154e2d)

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Oct 4, 2023
opensearch-project#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Oct 4, 2023
opensearch-project#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

do not set source env variables

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit that referenced this pull request Oct 4, 2023
#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 4, 2023
#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit dc6a7ec)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla pushed a commit that referenced this pull request Oct 4, 2023
#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.


(cherry picked from commit dc6a7ec)

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
willie-hung pushed a commit to willie-hung/OpenSearch-Dashboards that referenced this pull request Oct 5, 2023
opensearch-project#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Willie Hung <willie880201044@gmail.com>
SuZhou-Joe pushed a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Oct 7, 2023
opensearch-project#5134

Incorrectly uses the source of the code to pull down to BASE REF,
which would likely be `main` or `2.x`, etc.

It should be pulling down the PR branch. Cypress tests at the time
of merging were failing due to unrelated issue of disk allocation.

Not setting the env variables causes the workflow to rely on the default
values which is a return back to the original implementation and if the
env is set then it will be not empty.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants