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

[Bug]: Test result files are over-escaped in manifests now #4656

Closed
Swiddis opened this issue Apr 24, 2024 · 0 comments · Fixed by #4660
Closed

[Bug]: Test result files are over-escaped in manifests now #4656

Swiddis opened this issue Apr 24, 2024 · 0 comments · Fixed by #4660
Labels
bug Something isn't working

Comments

@Swiddis
Copy link
Contributor

Swiddis commented Apr 24, 2024

Describe the bug

It looks like my fix for #4569 was too aggressive, the fix is now over-escaping the url slashes and making it even harder to navigate...

Probably can fix it by adding / to the safe argument in quote_plus: urllib.parse.quote_plus(file, safe='/'). But I'm still not sure about the extra slash before cypress-screenshots in the URL.

To reproduce

Open any test manifest file with failing tests (example) and see the failing test link:

Example actual result:
https://ci.opensearch.org/ci/dbc/integ-test-opensearch-dashboards/2.14.0/7603/linux/x64/rpm/test-results/5851/integ-test/observabilityDashboards/with-security//cypress-screenshots%2Fplugins%2Fobservability-dashboards%2F6_notebooks.spec.js%2FTest+reporting+integration+if+plugin+installed+--+View+reports+homepage+from+context+menu+--+after+each+hook+%28failed%29.png

Expected behavior

Example expected result:
https://ci.opensearch.org/ci/dbc/integ-test-opensearch-dashboards/2.14.0/7603/linux/x64/rpm/test-results/5851/integ-test/observabilityDashboards/with-security/cypress-screenshots/plugins/observability-dashboards/6_notebooks.spec.js/Test+reporting+integration+if+plugin+installed+--+View+reports+homepage+from+context+menu+--+after+each+hook+%28failed%29.png

Screenshots

No response

Host / Environment

No response

Additional context

The original goal of #4569 (and I guess this issue too) is to make it possible to ctrl+click the test manifest URLs to quickly get logs and failure screenshots without too much manual fiddling.

Relevant log output

No response

@Swiddis Swiddis added bug Something isn't working untriaged Issues that have not yet been triaged labels Apr 24, 2024
@jordarlu jordarlu removed the untriaged Issues that have not yet been triaged label Apr 29, 2024
@Swiddis Swiddis changed the title [Bug]: Test result files are over-escaped now [Bug]: Test result files are over-escaped in manifests now Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants