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

Migrate query enhancement tests from functional repo to main #9048

Merged
merged 4 commits into from
Dec 14, 2024

Conversation

abbyhu2000
Copy link
Member

@abbyhu2000 abbyhu2000 commented Dec 12, 2024

Description

P0 PR for migrating the two query enhancement tests with its utils from functional repo to main. Modify cypress test workflow to allow this.

Need to merge this functional repo PR with this: opensearch-project/opensearch-dashboards-functional-test#1661

Built upon the comments in this PR: #8986

Issues Resolved

Changelog

  • skip

Check List

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

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid 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.

@abbyhu2000 abbyhu2000 force-pushed the feature/migrate-cypress-2 branch from 0425ffa to c3b8049 Compare December 12, 2024 18:50
@abbyhu2000 abbyhu2000 changed the title Feature/migrate cypress 2 Migrate query enhancement tests from functional repo to main Dec 12, 2024
Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid 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.

@abbyhu2000
Copy link
Member Author

Test flow link: Running this PR workflow with functional repo PR:opensearch-project/opensearch-dashboards-functional-test#1661

All cypress tests pass:

  • ciGroup 1-9: cypress tests in functional repo
  • ciGroup 10: 2 query enhancement tests in dashboard repo
  • ciGroup 11: dashboard sanity test in dashboard repo

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

Copy link
Member

@d-rowe d-rowe left a comment

Choose a reason for hiding this comment

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

Nice! I know this migration work is surprisingly challenging.

Do we have a PR prepared to remove these from ftr or will that be a followup?

Comment on lines +18 to +20
miscUtils.visitPage(
`app/data-explorer/discover#/?_g=(filters:!(),time:(from:'2015-09-19T13:31:44.000Z',to:'2015-09-24T01:31:44.000Z'))`
);
Copy link
Member

Choose a reason for hiding this comment

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

I know you're migrating these, but thinking that these type of hardcoded visits are good opportunities for utils in the future.

Ex.

DiscoverPage.visit({
    timeRange: {
        from: '2015-09-19T13:31:44.000Z',
        to: '2015-09-24T01:31:44.000Z'
    }
});

Copy link
Member

Choose a reason for hiding this comment

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

Seems we are still using the data from data-explorer. Will we do a follow-up to update the data? This is the one for query_enhancement #9006

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do a follow up to update the data after 9006 is merged

`app/data-explorer/discover#/?_g=(filters:!(),time:(from:'2015-09-19T13:31:44.000Z',to:'2015-09-24T01:31:44.000Z'))`
);

cy.waitForLoaderNewHeader();
Copy link
Member

Choose a reason for hiding this comment

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

Related to comment above: this can then be rolled into the Discover page visit helper so that no other consumers need to worry about load timing.

Again, no need to address for this PR. Just sharing some thoughts for longterm changes.

@joshuali925
Copy link
Member

do you have more background on why tests are moving from functional test repo to here? for example for other developers which repo should they use to add cypress tests

and is it necessary to add over one million lines of test data which doesn't appear to be from coming opensearch-project/opensearch-dashboards-functional-test#1661?

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid 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.

yarn.lock Outdated Show resolved Hide resolved
Signed-off-by: abbyhu2000 <[email protected]>
@abbyhu2000 abbyhu2000 force-pushed the feature/migrate-cypress-2 branch from 15b5d31 to b75f6f4 Compare December 13, 2024 23:27
@abbyhu2000
Copy link
Member Author

do you have more background on why tests are moving from functional test repo to here? for example for other developers which repo should they use to add cypress tests

and is it necessary to add over one million lines of test data which doesn't appear to be from coming opensearch-project/opensearch-dashboards-functional-test#1661?

@joshuali925 I think the future plan is to have dashboard cypress tests within the dashboard repo, and write future dashboard cypress test in dashboard repo. So in the future when writing features, we can add corresponding cypress tests in the same PR, then it can directly test the feature running those tests in a single PR. (Do not need to raise separate PR in functional repo to verify).

This PR just starts the process of migrating some old cypress tests back to dashboard repo.

cc: @ashwin-pc

@abbyhu2000
Copy link
Member Author

do you have more background on why tests are moving from functional test repo to here? for example for other developers which repo should they use to add cypress tests

and is it necessary to add over one million lines of test data which doesn't appear to be from coming opensearch-project/opensearch-dashboards-functional-test#1661?

both logstash data and discover data are data explorer plugin coming from functional repo. I will raise a follow up PR to clean the data and use the query enhancement data once this PR is merged: #9006

Signed-off-by: abbyhu2000 <[email protected]>
@github-actions github-actions bot added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Dec 13, 2024
Signed-off-by: abbyhu2000 <[email protected]>
@abbyhu2000
Copy link
Member Author

For some reasons, auto generate changelog mechanism does not work properly. Have manually added a changelog

@ananzh
Copy link
Member

ananzh commented Dec 14, 2024

@abbyhu2000 will do a follow up to update data and clean out. Approve this to unblock CI and tests merging.

@abbyhu2000 abbyhu2000 merged commit 442e11e into main Dec 14, 2024
66 of 67 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-9048-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 442e11eaa82f32eeba2e1a0e75dd4a70a2cde076
# Push it to GitHub
git push --set-upstream origin backport/backport-9048-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-9048-to-2.x.

ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Dec 16, 2024
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Dec 16, 2024
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Dec 16, 2024
LDrago27 pushed a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Dec 16, 2024
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Dec 18, 2024
ananzh added a commit that referenced this pull request Dec 18, 2024
* Follow up on #9048 by updating data and utilities

Feature branch PRs:
#9038
#9006

Signed-off-by: Anan <[email protected]>

* fix comment

Signed-off-by: Anan <[email protected]>

* update path and add utility from suchit commit

Signed-off-by: Anan <[email protected]>

* update the path to run sample test

Signed-off-by: Anan <[email protected]>

* fix PR comments

Signed-off-by: Anan <[email protected]>

* skip tests before enable workspace

Signed-off-by: Anan <[email protected]>

---------

Signed-off-by: Anan <[email protected]>
@ananzh ananzh added the discover_2.0-test Issues that are specific to the Discover 2.0 testing initiative label Dec 18, 2024
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x discover_2.0-test Issues that are specific to the Discover 2.0 testing initiative distinguished-contributor failed backport Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants