cypress-example-recipes icon indicating copy to clipboard operation
cypress-example-recipes copied to clipboard

Remove non-working cross-origin download examples

Open jennifer-shehane opened this issue 4 years ago • 4 comments

  • Remove example of cross-origin downloads as these are not supported in Cypress. These were falsely passing since we do not clear downloads in between each test, so they were just verifying that the previous download was successful in the local downloads.
  • Remove chromeWebSecurity: false from tests - we should never recommend this.
  • Updated the 'find file' test's glob to find the correct file.
  • Add note in readme warning that testing downloaded files cross-origin is not supported.

From original discussion in https://github.com/cypress-io/cypress/pull/14749#issuecomment-768378556

  1. I ran the example-recipe of the file downloads and some of the (download) events are showing up in a later test as pending (not the test where the file downloaded was triggered). Maybe this is accurate to when the download event actually happened? But it sure is confusing. :/ Screen Shot 2021-01-27 at 10 44 56 AM

Those downloads are cross-origin, which is supposed to work with chromeWebSecurity: false, but I think this is actually revealing that they aren't working at all. Looking at the downloads bar in Chrome, it says they're failing with a "Network Error". The tests only pass because the same-origin tests above them download the files and the cross-origin ones read them. If you clear the downloads and run a single cross-origin test, it will fail.

My two takeaways from this are:

  1. We shouldn't suggest using chromeWebSecurity: false for this (or maybe for anything), so I think we should remove those examples from the recipe. They don't really demonstrate anything that's unique from the same-origin examples anyway. Maybe once we have multi-domain support, we can explore some way to support cross-origin downloads, though I'm not sure it's really possible.

  2. The false positives highlight the danger of only clearing downloads between runs and not between tests. If a user tests the same file in multiple tests, which is a fairly likely chance, there's a risk of such a false positive. We should think about changing it so downloads are cleared between tests (with a config option to disable).

jennifer-shehane avatar Jan 28 '21 04:01 jennifer-shehane

Sorry Gleb, I now realize I wasn't running the cross-origin server (npm run start:second) when running the tests and that was causing the "Network Error" download failures. So the cross-origin ones do pass with chromeWebSecurity: false. They also don't exhibit the UI issues since the downloads complete successfully.

I still don't like recommending chromeWebSecurity: false. It's one thing if users discover and use it or even if we mention it briefly in an issue or the docs saying "this could work but with a big caveat." When we put it in a recipe, it's like we're endorsing it. Also, do we need an entire section of the recipe devoted to cross-origin downloads? It's exactly the same code as the same-origin tests. The part that's different is in another file. What about adding a comment to the readme and the spec file saying "Use chromeWebSecurity: false at your own risk to enable cross-origin downloads"?

chrisbreiding avatar Jan 28 '21 14:01 chrisbreiding

We can recommend or not - but it is there. Users can decide what to do, they don't have to use it, but if they need to test cross-origin, we better show them the right way. Endorsement - I am not sure, we can always say "hey, as always, use disabling chrome web security at your own risk"

bahmutov avatar Jan 28 '21 14:01 bahmutov

@bahmutov Sorry I misinterpreted some things from the original comment about support.

I think that if we are going to discourage turning off Chrome Web Security then we need to really outline why we don't advise it because currently our docs are just like - 'need to turn it off? here's how'. https://docs.cypress.io/guides/guides/web-security.html#Disabling-Web-Security

If we had a place in the docs to point to, then I would advise always linking to that section of the docs every time in recipes/blogs that we tell them to turn off chrome web security.

jennifer-shehane avatar Jan 29 '21 04:01 jennifer-shehane

@bahmutov wrote:

I could see the potential for confusion when not clearing the downloads before each test, but I also do not think deleting them before the test works, because the users might want to look at all downloaded files later.

I disagree with this reason. Cypress already cleans up cookies before each test and I think files are in the same category. Moreover, if a developer wants to analyze downloaded files, s/he can do so after running the test because files are only deleted before tests, and never after tests. As a result, I suggest deleting everything before a test start.

DamienCassou avatar Feb 04 '21 10:02 DamienCassou