-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: cy.route swallows the error about missing fixture #7818 #7983
Conversation
Thanks for taking the time to open a PR!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@przemuh There's a failure with this implementation when run through our cypress-example-kitchensink
repo. https://circleci.com/gh/cypress-io/cypress/395839
You should be able to clone the repo and run it in your branch with yarn start
, add the project and see the failure from the files
spec. Can you track down what introduced this failure?
@jennifer-shehane Please check it now, it should be ok :) The problem was in |
6cfe30f
to
432cd31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This null.json
fixture should have already worked per #5562. But weirdly enough, it seems that we only fixed when this it's passed via fx:
there: https://github.com/cypress-io/cypress/blob/develop/packages/driver/cypress/integration/commands/xhr_spec.js#L1870:L1870
I opened an issue to detail the null.json error here: #8010
Dismissing my previous review for more thorough code review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super, tests were added, so that's good. I just need an answer to my question for
return cy.fixture(options.response.replace(fixturesRe, '')).then(() => route())
Hey @przemuh, there were some requests for changes. It should be good to go after those are addressed. Do you have time to address them? |
Hey @jennifer-shehane. This week I am on vacations🏖 without access to my computer...I will be more than happy to address all comments but around Sunday evening CET. I hope that it will not be a problem 😅 |
…e does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super, I think this is fine now
I've identified that this PR introduced a regression - where the fixture files take longer to load, especially at large fixture file sizes, sometimes timing out. Detailed here: #8181 |
@jennifer-shehane I am really sorry about that :( I hope you will forgive me ;) Here is a pull request with fix/revert: #8215 |
Cannot read property '__error' of null
error thrown when providingnull.json
tocy.fixture()
#8010User facing changelog
fixture
command which raised an error fornull.json
. BTW. did you know that if the fixture contains following key in the returned object__error
thency.fixture
throws an error :) ?Additional details
Why was this change necessary?
To provide better error message in case of missing fixture or problem with parsing
What is affected by this change?
cy.route
callscy.fixture
if response meets criteria of fixture shortcutfx:
|fixture:
. With this additional check we can raise an error even before HTTP request is sent.How has the user experience changed?
If fixture file doesn't exists, or there is a problem with fixture,
cy.route
will raise an error.Before:
![image](https://user-images.githubusercontent.com/3865478/87590038-74a57680-c6e6-11ea-977a-cd530f689a8f.png)
Now:
![image](https://user-images.githubusercontent.com/3865478/87591323-6eb09500-c6e8-11ea-9644-329d1a529318.png)
PR Tasks