-
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
Decaff driver (final) #7430
Decaff driver (final) #7430
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
It seems that flaky test happened. |
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.
Thanks again for doing this! We're almost there. 😄
@@ -374,6 +373,7 @@ Body: <!DOCTYPE html> | |||
</body> | |||
</html> | |||
|
|||
|
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.
but this same error adds a newline to the snapshot snapshot due to an update in error_messages, shouldn't be double newlines here I think.
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 problem needs a research.
When I reverted the change and run the tests on my local Ubuntu, it works. But when I pushed it, it fails.
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.
I still don't know why it happened. But the fix was easy.
@@ -246,7 +246,6 @@ Headers: { | |||
} | |||
Body: Service Unavailable | |||
|
|||
|
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 appears to be remove a newline from the error message due to an update in error_messages.
docsUrl: 'https://on.cypress.io/request', | ||
} | ||
}, | ||
status_invalid (obj) { |
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.
There's something about the cyStripIndent
function that's not the same as previous spacing. Something about it changes the error message in the snapshotted values.
Another flaky failure. |
|
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.
Perfect! If this is all the coffeescript out, I'm not sure if we had any code around transpiling that could be removed, but this could be removed now if so.
@jennifer-shehane Unfortunately, there are a lot of coffeescript codes in |
Part of #2690
Final decaff of the
driver
package.NEVER SQUASH THIS PR.