cbioportal-frontend icon indicating copy to clipboard operation
cbioportal-frontend copied to clipboard

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

Open pappde opened this issue 1 year ago • 4 comments

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

pappde avatar Jun 18 '24 22:06 pappde

Deploy Preview for cbioportalfrontend ready!

Name Link
Latest commit 2a07708c327e3e0f3a077e6cc6fad0f986e1d284
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.

netlify[bot] avatar Jun 18 '24 22:06 netlify[bot]

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

pappde avatar Jun 22 '24 00:06 pappde

@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(){...})

alisman avatar Jun 25 '24 17:06 alisman

@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 avatar Jun 25 '24 22:06 pappde

@alisman Hi, I am still experiencing this issue on a Windows system, seems like this commit hasn't been merged to master yet. Do you have any plans regarding this?

GaborLopusni avatar Jun 15 '25 17:06 GaborLopusni