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

fix: Adding an existing command with Cypress.Commands.add() will throw an error #18587

Merged
merged 24 commits into from
Nov 8, 2021

Conversation

davidmunechika
Copy link
Contributor

@davidmunechika davidmunechika commented Oct 21, 2021

User facing changelog

Attempting to add an existing built-in Cypress command using Cypress.Commands.add() will throw an error, indicating that Cypress.Commands.overwrite() should be used instead to overwrite the behavior of existing commands. This is a breaking change since errors will now be thrown if users had previously added any new commands which already existed in Cypress.

Additional details

Previously, Cypress would not warn you when you are trying to add a command that already exists in Cypress. This could lead to unexpected behavior with the command since it is unclear which would take precedence.

Now, if a user tries to add a command that already exists (such as Cypress.Commands.add('get', ...)), Cypress will throw an error indicating that overwrite should be used in these situations instead.

Note: the implementation in this PR will NOT throw if a user attempts to add a duplicate custom command that they have already defined. This only checks for collisions with core Cypress commands.

PR Tasks

@davidmunechika davidmunechika self-assigned this Oct 21, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 21, 2021

Thanks for taking the time to open a PR!

@davidmunechika davidmunechika changed the title Adding an existing command with Cypress.Commands.add() will throw an error fix: Adding an existing command with Cypress.Commands.add() will throw an error Oct 21, 2021
@cypress
Copy link

cypress bot commented Oct 21, 2021



Test summary

8917 0 106 4Flakiness 3


Run details

Project cypress
Status Passed
Commit 458d142
Started Nov 8, 2021 2:38 PM
Ended Nov 8, 2021 2:49 PM
Duration 11:10 💡
OS Linux Debian - 10.9
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/proxy-logging-spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code
commands/net_stubbing_spec.ts Flakiness
1 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
e2e/redirects_spec.js Flakiness
1 redirection > meta > binds to the new page after a timeout

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@davidmunechika davidmunechika marked this pull request as ready for review October 26, 2021 14:03
@davidmunechika davidmunechika requested a review from a team as a code owner October 26, 2021 14:03
@davidmunechika davidmunechika requested review from jennifer-shehane and removed request for a team October 26, 2021 14:03
@jennifer-shehane jennifer-shehane requested review from chrisbreiding and BlueWinds and removed request for jennifer-shehane October 26, 2021 15:06
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
@chrisbreiding chrisbreiding self-requested a review October 26, 2021 15:30
chrisbreiding
chrisbreiding previously approved these changes Oct 27, 2021
@jennifer-shehane
Copy link
Member

@davidmunechika @chrisbreiding Is there a way to get a better stack trace on this or a code frame where it is called?

I guess my main concern is that the use case for this may be that Cypress adds an .attachFile command, and the command where this error is thrown is actually defined within a plugin, so when they get this message - they may not know where to look for this definition since they didn't write it.

Screen Shot 2021-10-28 at 9 38 30 AM

@BlueWinds
Copy link
Contributor

Are we sure this is a fix rather than a breaking change?

Also, should probably update the user-facing changelog with the fact that this only applies to builtin cypress commands (and they can continue to use cy.add() to overwrite their own commands, a'la in beforeEach()).

BlueWinds
BlueWinds previously approved these changes Oct 28, 2021
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

@jennifer-shehane We improved the error stack so it now points to the user's code and shows a code frame.

I agree with @BlueWinds that this should probably be considered a breaking change and noted as such in the changelog.

@jennifer-shehane jennifer-shehane dismissed their stale review November 5, 2021 15:33

Dismissing my review

@davidmunechika davidmunechika changed the base branch from develop to 9.0-release November 8, 2021 16:17
@jennifer-shehane jennifer-shehane merged commit b735978 into 9.0-release Nov 8, 2021
@jennifer-shehane jennifer-shehane deleted the issue-18572-add-existing-command branch November 8, 2021 20:58
tgriesser added a commit that referenced this pull request Nov 18, 2021
* develop: (329 commits)
  chore: Update Chrome (stable) to 96.0.4664.45 (#18931)
  fix: Loading of specs with % in the filename (#18877)
  chore: refactor `create` into class `$Cy` (#18715)
  chore: Update Chrome (beta) to 96.0.4664.45 (#18891)
  fix: flaky `system-tests-firefox` job (#18848)
  release 9.0.0
  feat: ensure major release
  have conduit app wait on localhost:3000
  fix install-required-node
  use --legacy-peer-deps
  feat: ensure major release
  fix darwin node install
  chore(driver): fix integration test retry configuration (#18643)
  feat(deps): update dependency electron to v15 🌟 (#18317)
  chore: Bind this correctly when setting response headers with cy.route() (#18859)
  feat: create config package for config validation (#18589)
  chore: patch `winston` to suppress `padLevels` warning (#18824)
  chore: test out major release build
  fix: remove outdated npm registry links (#18727)
  fix: Adding an existing command with `Cypress.Commands.add()` will throw an error (#18587)
  ...
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.

Cypress.Command.add for a command that is a Cypress command doesn't warn/error
4 participants