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

Remove non-working cross-origin download examples #631

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

Conversation

jennifer-shehane
Copy link
Member

@jennifer-shehane jennifer-shehane commented Jan 28, 2021

  • 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 cypress-io/cypress#14749 (comment)

  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 jennifer-shehane changed the title Remove remote downloads, fix falsely passing 'find file' test Remove non-working cross-origin download examples Jan 28, 2021
Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. downloads from cross-origin do work with chromeWebSecurity: false. Here is a video of me doing just that!
remote-file-download.mp4
  1. 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. So I would keep it as is. After all, the users can write a task to clear it.
  2. we absolutely HAVE to show the cross-origin recipe, even if it works in Chrome only. It is self-defeating to hide it. First, it is possible. Second, users will discover it. Third, users who do not discover it will keep asking about it.

I think what we can do to clearly show it here is to split the spec into same-origin vs cross-origin and add comments explaining the situation. If you want, I can take over this PR to update it.

@chrisbreiding
Copy link
Contributor

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"?

@bahmutov
Copy link
Contributor

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"

@jennifer-shehane
Copy link
Member Author

@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.

@DamienCassou
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants