search

cBioPortal / cbioportal

cBioPortal for Cancer Genomics
https://cbioportal.org
GNU Affero General Public License v3.0
578 stars 440 forks source link

E2etests Screenshots - invalid path names block git checkout in Windows environment #10836

Open pappde opened 2 weeks ago

pappde commented 2 weeks ago

REPRO:

  1. clone the repo on a Windows machine
  2. try to checkout
  3. observe it fails due to invalid filenames on Windows (due to ">" and ":") due to 4 screenshots for e2etests:
error: invalid path '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'
error: invalid path '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'
error: invalid path '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'
error: invalid path 'end-to-end-test/remote/screenshots/reference/download_tab_-_nsclc_tcga_broad_2016_for_query_egfr:_mut=t790m_amp_element_chrome_1600x1000.png'
error: invalid path 'end-to-end-test/remote/screenshots/reference/msk_impact_2017_query_stk11:homdel_mut_element_chrome_1600x1000.png'

EXPECTED:

  1. checkout succeeds

NOTES 1) Not sure if these characters are allowed in Mac filenames either. Also avoiding '>' probably a good idea even in linux systems since it could cause some problems with shell commands.

2) Would also need some trigger or check on commits to prevent new files being introduced with these characters: \, /, :, *, ?, ", <, >, |

3) I realize that WSL is recommended for development in a Windows environment, but this may be an easy fix to enable that scenario without WSL. However, not sure if these filenames are explicitly referenced in a database for e2etests, or how they are pulled. Aliasing or escaping may be necessary to ensure existing references still work.

pappde commented 2 weeks ago

Fixed in PR #4927.

Possible followup

  1. Could split off a new issue for adding a pre-commit trigger to prevent invalid path characters in filenames.
  2. Review existing e2etests. If there is one that references the renamed file, it would need to be updated to the new filename.
pappde commented 1 week ago

Fixed in PR #4927. Modified getScreenshotName() to replace all invalid path characters.