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

e2eTests - Screenshots/Reference: fixed files with invalid paths preventing repo checkout on Windows environment #4927

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pappde
Copy link
Contributor

@pappde pappde commented Jun 18, 2024

BACKGROUND

Without this commit, it is not possible to checkout the repo in a Windows environment (and possibly others) There are 5 screenshots under e2etests that have invalid path characters in the name. Git on Windows will simply abort a checkout in this scenario. Some of the files have a '>' in the name which is good idea to avoid for command line work anyways.

DESCRIPTION

In this PR, 4 of the screenshots with invalid paths have been deleted. They already have corrected versions. One of the screenshots was renamed (the ":" to a "_" following the convention for the other files).

Deleted Files with other versions already existing:

  • end-to-end-test/local/screenshots/reference/shows_`value_>8.00`_in_figure_legend_and_indicates_sub-threshold_data_points_in_plot_element_chrome_1600x1000.png
  • end-to-end-test/local/screenshots/reference/when_option_deselected,_hides_`value_>8.00`_in_figure_legend_and_sub-threshold_data_point_indicators_in_waterfall_plot_element_chrome_1600x1000.png
  • end-to-end-test/local/screenshots/reference/when_option_deselected,_hides_`value_>8.00`_in_figure_legend_and_sub-threshold_data_points_in_plot_element_chrome_1600x1000.png'
  • end-to-end-test/remote/screenshots/reference/msk_impact_2017_query_stk11:homdel_mut_element_chrome_1600x1000.png

Renamed Files:

  • end-to-end-test/remote/screenshots/reference/download_tab_-_nsclc_tcga_broad_2016_for_query_egfr:_mut=t790m_amp_element_chrome_1600x1000.png
    CHANGE: renamed the : to _
    NEW NAME: download_tab_-_nsclc_tcga_broad_2016_for_query_egfr__mut=t790m_amp_element_chrome_1600x1000.png

ISSUE TRACKER

Fixes cBioPortal/cbioportal#10836

Tests

No tests have been added. Arguably, a pre-commit trigger could be added to check filenames for invalid path characters, or a test, but this is likely a rare scenario so a low priority task could be added.

Also, not sure where references to the screenshots used in e2etests are stored. At least the one reference to the renamed file may need to be updated.

Any screenshots or GIFs?

n/a

Notify reviewers

TBD

Copy link

netlify bot commented Jun 18, 2024

Deploy Preview for cbioportalfrontend ready!

Name Link
🔨 Latest commit 2a07708
🔍 Latest deploy log https://app.netlify.com/sites/cbioportalfrontend/deploys/669051442851600008d653ca
😎 Deploy Preview https://deploy-preview-4927--cbioportalfrontend.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pappde
Copy link
Contributor Author

pappde commented Jun 22, 2024

I see the CI failed on e2e tests but these failures are the same as the master branch.

@pappde pappde force-pushed the fix/invalid-paths branch from a8b9d96 to 780bb9e Compare June 24, 2024 19:06
@alisman
Copy link
Collaborator

alisman commented Jun 25, 2024

@pappde thanks for doing this. These filenames are autogenerated based on the describe/it labels in specs. So to fix this:

  1. change the labels in the spec files to get rid of the problem chars
  2. push and tests will run in CI env, which will create new reference screenshots that we can download and commit to repo (and delete old ones)
    Happy to help with this. Maybe you could just go change the labels

it('LABEL IS THIS STRING HERE', function(){...})

@pappde
Copy link
Contributor Author

pappde commented Jun 25, 2024

@alisman That was very helpful. I tracked down where the screenshot filenames are generated from the test name, and updated it to fix all invalid path characters. That commit has been added to this PR.

For the 4 screenshots above that are marked "Deleted Files with other versions already existing", I verified that the test names were already updated to reference the renamed file. So the old files (with invalid characters) were orphans, safely removed.

For the 5th screenshot above marked "Renamed Files", this new commit fixes it so the test name matches the corrected filename. There is no need to change any test names.

I verified this change doesn't impact any of the existing screenshot files.

pappde and others added 2 commits July 11, 2024 17:38
…with invalid characters in name for Windows and possibly other environments.

- 4 of them already have copies with fixed filenames that exists
- 1 file renamed the ':' to '_'

The original paths:
'end-to-end-test/local/screenshots/reference/shows_`value_>8.00`_in_figure_legend_and_indicates_sub-threshold_data_points_in_plot_element_chrome_1600x1000.png'
'end-to-end-test/local/screenshots/reference/when_option_deselected,_hides_`value_>8.00`_in_figure_legend_and_sub-threshold_data_point_indicators_in_waterfall_plot_element_chrome_1600x1000.png'
'end-to-end-test/local/screenshots/reference/when_option_deselected,_hides_`value_>8.00`_in_figure_legend_and_sub-threshold_data_points_in_plot_element_chrome_1600x1000.png'
'end-to-end-test/remote/screenshots/reference/download_tab_-_nsclc_tcga_broad_2016_for_query_egfr:_mut=t790m_amp_element_chrome_1600x1000.png'
'end-to-end-test/remote/screenshots/reference/msk_impact_2017_query_stk11:homdel_mut_element_chrome_1600x1000.png'
…s with '_'.

This currently only affects one file and fixes it so generated screenshot will now match the reference screenshot: end-to-end-test/remote/screenshots/reference/download_tab_-_nsclc_tcga_broad_2016_for_query_egfr:_mut=t790m_amp_element_chrome_1600x1000.png

NOTE: there are four other files that previously had invalid path characters, but they have already been renamed and the tests already updated
@alisman alisman force-pushed the fix/invalid-paths branch from 47dc44e to 2a07708 Compare July 11, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2etests Screenshots - invalid path names block git checkout in Windows environment
3 participants