-
Notifications
You must be signed in to change notification settings - Fork 884
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
Add query enhancement cypress tests to Github CI #8703
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
@@ -85,8 +85,7 @@ | |||
"release_note:generate": "scripts/use_node scripts/generate_release_note", | |||
"cypress:run-without-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --headless --env SECURITY_ENABLED=false", | |||
"cypress:run-with-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --headless --env SECURITY_ENABLED=true,openSearchUrl=https://localhost:9200,WAIT_FOR_LOADER_BUFFER_MS=500", | |||
"osd:ciGroup10": "echo \"dashboard_sanity_test_spec.js\"", | |||
"osd:ciGroup11": "echo \"apps/vis_builder/*.js\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed "osd:ciGroup11": "echo \"apps/vis_builder/*.js\"",
because vis_builder tests are already running in ciGroup1 as defined here https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/package.json#L28
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8703 +/- ##
==========================================
- Coverage 60.84% 60.81% -0.03%
==========================================
Files 3793 3793
Lines 90486 90486
Branches 14212 14212
==========================================
- Hits 55057 55031 -26
- Misses 31939 32009 +70
+ Partials 3490 3446 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
Cypress runner: CiGroup10 is testing query enhancement tests that are added in this functional repo PR: opensearch-project/opensearch-dashboards-functional-test#1598 All passing except for CiGroup 6, i think currently ciGroup6 is failing on main, should not be relate to this PR. |
|
||
jobs: | ||
cypress-tests: | ||
runs-on: ubuntu-latest | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
group: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] | ||
include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work and help organize?
matrix:
config: [standard, query_enhanced, dashboard]
group: [1, 2, 3, 4, 5, 6, 7, 8, 9]
include:
- config: query_enhanced
group: 10
test_location: ftr
- config: dashboard
group: 11
test_location: source
exclude:
- config: query_enhanced
group: [1,2,3,4,5,6,7,8,9]
- config: dashboard
group: [1,2,3,4,5,6,7,8,9]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, using matrix to create the right config could make it shorter. but i do see a benefit of making it this granular as it we have more control. as it leave spaces in the future where we migrate the tests from ftr to this repo
QQ: so for writing tests in ciGroup10, do we need add some code to create a workspace? and test in the workspace? |
i
I think i asked @ashwin-pc during standup and he responded that right now we only covers the flows on discover pages. Do we want to add test to include extra workflows like creating the workspace? or we only test what discover page covers. |
test_location: ftr | ||
- group: 3 | ||
config: standard | ||
test_location: ftr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a variable FTR_PATH, should we use this/change FTR_PATH to something that makes more sense to usage and add the other locations? or should we get rid of the FTR_PATH variable
@@ -42,18 +40,65 @@ env: | |||
COMMENT_TAG: '[MANUAL CYPRESS TEST RUN RESULTS]' | |||
COMMENT_SUCCESS_MSG: ':white_check_mark: Cypress test run succeeded!' | |||
COMMENT_FAILURE_MSG: ':x: Cypress test run failed!' | |||
LATEST_VERSION: '2.17.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe in an inline comment on why we are using this. i believe it was because of issues trying to use main and install plugins for github virtual env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing we could also do is actually use to make this dynamic is rely on git tags. every release infra will cut a tag for the release in all the repos post release and then we use that to make a github release. we could potentially just get the highest version tag from our repo and assume that is the highest release version of OpenSearch that we can download
- name: Setup spec files for Dashboards tests | ||
if: ${{ inputs.specs == '' && matrix.group > 9 }} | ||
# Run tests based on configuration | ||
- name: Run FT repo tests with no query enhancements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: historically we called this FTRepo.
and I think the specification with no query enhancements
might be unnecessary. we should go with the actual one run the query enhancements to say with query enhancements
incase we want to follow this format in the future and add different type of situations.
# Run tests based on configuration | ||
- name: Run FT repo tests with no query enhancements | ||
if: matrix.test_location == 'ftr' && matrix.config == 'standard' | ||
uses: cypress-io/github-action@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we want to bump the cypress github action if available and works? might have improvements https://github.com/cypress-io/github-action/blob/master/CHANGELOG.md. but i see somethings in our file will need to change so maybe not worht it at this time
- name: Extract OpenSearch | ||
if: matrix.test_location == 'ftr' && matrix.config == 'query_enhanced' | ||
run: | | ||
tar -xzf opensearch-*.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to untar this file and pass the param to call it a specific name so line 238 doesn't have to be a specific version
if: matrix.test_location == 'ftr' && matrix.config == 'query_enhanced' | ||
run: | | ||
/bin/bash -c "./opensearch-2.17.0/opensearch-tar-install.sh &" | ||
sleep 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confused about this one.
wait-on should be actually a better resolution to this: https://github.com/cypress-io/github-action?tab=readme-ov-file#wait-on
are you seeing issues with that?
container: | ||
image: docker://opensearchstaging/ci-runner:ci-runner-rockylinux8-opensearch-dashboards-integtest-v2 | ||
options: --user 1001 | ||
env: | ||
START_CMD: ${{ matrix.config == 'query_enhanced' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like this concept of config
because it actually follows the format well with how OSD functional tests are structured!
only nit I have here is that this actually is a little bit hard to read. and any fixes or additional updates to this will require us to know to update both places.
so i think minimum we should consider the logic being where we append extra configurations if the matrix config is query enhanced instead of having two.
if that is not good enough then it might be time to start sourcing a .env config file that stores this information so that we can modify this stuff and make this file more approachable.
config: standard | ||
test_location: ftr | ||
- group: 4 | ||
config: standard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: default
has been used for this type of situation or undefined and we automatically apply the default config
IFS="," read -a SPEC_ARRAY <<< $(yarn --silent osd:ciGroup${{ matrix.group }}) | ||
DASHBOARDS_SPEC='' | ||
for i in "${SPEC_ARRAY[@]}"; do | ||
DASHBOARDS_SPEC+="cypress/integration/core_opensearch_dashboards/${i}," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we desperately need to be able to tag cypress test so that when we write them inhouse the test can decide which workflow it will run in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like the proposal. i mentioned this in the comments moving towards the config route is actually more in line with how OSD functional tests are so the system is better built for that. So nice call!
Some comments though related to future proving the solution
Description
Add query enhancement cypress tests workflow to github CI to test discover 2.0 functionalities:
The tests can be added under folder
cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/query_enhancement
in the functional repo, and will be run in ciGroup10 in the CI.Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration