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

Integration tests for broken safari link #940

Merged

Conversation

leanneeliatra
Copy link
Contributor

@leanneeliatra leanneeliatra commented Oct 27, 2023

Description

Tests that when the link is copyed in Safari, it is copyed successfully and can be visited.
When visiting Discover > Copy link

Issues Resolved

Integration tests for the associated bugfix:
[BUG] Share > Copy link broken in Safari #1716

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ruanyl
Copy link
Member

ruanyl commented Oct 30, 2023

Hi @leanneeliatra, could you convert this PR to "draft" if it is "IN PROGRESS"?

@leanneeliatra leanneeliatra changed the title Integration tests for broken safari link DRAFT: Integration tests for broken safari link Oct 31, 2023
@leanneeliatra leanneeliatra force-pushed the copy-link-in-safari-test branch from b0ab7d2 to d97e891 Compare November 6, 2023 07:39
@leanneeliatra leanneeliatra reopened this Nov 6, 2023
@leanneeliatra
Copy link
Contributor Author

This PR was closed accidentally, it is now reopened.

@leanneeliatra leanneeliatra changed the title DRAFT: Integration tests for broken safari link Integration tests for broken safari link Nov 6, 2023
@leanneeliatra
Copy link
Contributor Author

Tests are passing locally
Uploading Screenshot 2023-11-06 at 15.08.48.png…

@leanneeliatra
Copy link
Contributor Author

leanneeliatra commented Nov 6, 2023

For testing locally I am testing this in safari using: https://docs.cypress.io/guides/guides/launching-browsers#WebKit-Experimental (WebKit is available only via an experimental feature of Cypress). Given that this is experimental, it cannot be merged into the CI pipeline.

@leanneeliatra
Copy link
Contributor Author

Cypress tests passing locally
Screenshot 2023-11-13 at 20 40 33

Screenshot 2023-11-13 at 20 40 41

Signed-off-by: leanneeliatra <[email protected]>
@leanneeliatra
Copy link
Contributor Author

leanneeliatra commented Nov 14, 2023

This can now be reviewed please
The cypress workflow check Snapshot based E2E Cypress tests workflow / Run Cypress E2E tests (pull_request)
doesn't seem to be passing but as you can see above they are, so I am a bit confused there, can anyone direct please?
@Hailong-am @xluo-aws @SuZhou-Joe @ruanyl

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Nov 15, 2023

This can now be reviewed please The cypress workflow check Snapshot based E2E Cypress tests workflow / Run Cypress E2E tests (pull_request) doesn't seem to be passing but as you can see above they are, so I am a bit confused there, can anyone direct please? @Hailong-am @xluo-aws @SuZhou-Joe @ruanyl

For the snapshot based test, it should has something to do with the Node version upgrade to v18 and you can ignore that for now.

Some action items maybe:

  • For the file location, I suppose it should be located in cypress/integration/core-opensearch-dashboards instead of security-dashboards as it is a common issue in old version of OuiCopy. @leanneeliatra Could you help confirm that?
  • For now FTRepo only runs integration tests in Chrome / Firefox without Safari, I suppose we may need to setup flow in Safari browser @ruanyl @kavilla I'd like to hear your advice about this.
  • I am a little bit confused why the flow Security Release tests workflow in Bundled OpenSearch Dashboards was unable to run even though there are some changes matching its filter configuration, set up a issue to track.

@leanneeliatra
Copy link
Contributor Author

leanneeliatra commented Nov 20, 2023

Thank you @SuZhou-Joe for the feedback, much appreciated.

  1. Yes the file location suggested seems like a better place for the new test, I have added it to that location and changed the name to be more descriptive.
    cypress/integration/core-opensearch-dashboards/opensearch-dashboards/dashboard_share_copy_link_test.js
  2. It would be great to get guidance on this. I have tested in Safari and the copy link works as expected but further guidance on whether we need to do something with the test for merging into the CI would be great. @ruanyl @kavilla
  3. Thanks for opening the ticket with regard to Security Release tests workflow in Bundled OpenSearch Dashboards

@leanneeliatra
Copy link
Contributor Author

This can now be reviewed please if anyone has a moment. Thanks.
cc @Hailong-am @xluo-aws @ruanyl

@Hailong-am
Copy link
Collaborator

waiting for opensearch-project/security-dashboards-plugin#1633 be merged

@leanneeliatra
Copy link
Contributor Author

These can now be merged @Hailong-am, thank you for your assistance.

@Hailong-am Hailong-am merged commit a6e67f9 into opensearch-project:main Nov 29, 2023
33 of 38 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 29, 2023
* copy link lint fix

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

* testing visit to copyed link works

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

* copy from clipboard and visit copied link

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

* Update copy_link.js

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

* renamed copy_link and moved file location

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

---------

Signed-off-by: [email protected] <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
(cherry picked from commit a6e67f9)
Hailong-am pushed a commit that referenced this pull request Nov 30, 2023
* copy link lint fix

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

* testing visit to copyed link works

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

* copy from clipboard and visit copied link

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

* Update copy_link.js

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

* renamed copy_link and moved file location

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

---------

Signed-off-by: [email protected] <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
(cherry picked from commit a6e67f9)

Co-authored-by: leanneeliatra <[email protected]>
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.

4 participants