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

Assertions passed with a number argument should be conditionally be casted as a string on DOM string accessors #7314

Closed
brian-mann opened this issue May 12, 2020 · 5 comments · Fixed by #7315
Assignees
Labels
type: unexpected behavior User expected result, but got another

Comments

@brian-mann
Copy link
Member

brian-mann commented May 12, 2020

Current behavior:

Passing a number to the the following assertions always fails, but casting it to a string passes.

  • have.text
  • have.id
  • have.data
  • have.value
  • have.attr

Desired behavior:

Numbers should automatically be casted to a string when necessary.

Test code to reproduce

// app.html
<div>5</div>

// app.spec.js
cy.get("div").should("have.text", 5)       // fails
cy.get("div").should("have.text", `${5}`)  // passes

And other combinations listed above ^^

Versions

4.5.0

@brian-mann brian-mann added the type: unexpected behavior User expected result, but got another label May 12, 2020
@flotwig
Copy link
Contributor

flotwig commented May 12, 2020

I think the assertion in the OP should throw an error so the user can fix the problem on their side, because automatically stringifying the argument has some other, confusing, implications.

How will this auto-casting handle precision? How will it handle limited float precision? Should this pass too?

<div>1.50</div>

// will this pass? String(1.50) === '1.5' !== '1.50'
cy.get("div").should('have.text', 1.50)

// or...

<div>1</div>

// will this pass? 
// String(1.00000000000000000000000000000001) === '1'
cy.get("div").should('have.text', 1.00000000000000000000000000000001)

Also, numbers above Number.MAX_SAFE_INTEGER (think credit card numbers, or serial numbers) will lose precision when converted to floats in the same way and cause similarly unexpected behavior. Also, numbers beginning with 0 will be converted to octals... just seems like it should throw an error so the user can decide the most correct course of action.

@brian-mann brian-mann changed the title Assertions passed with a number argument should automatically be casted as a string Assertions passed with a number argument should be conditionally be casted as a string on DOM string accessors May 12, 2020
@cypress-bot cypress-bot bot added the stage: work in progress There is an open PR for this issue [WIP] label May 12, 2020
@brian-mann
Copy link
Member Author

brian-mann commented May 16, 2020

This is a valid point - but I think because this is already in an assertion - there's no reason to double throw here. The worst thing that can happen is that our conversion misses and fails for another reason. But not doing the string conversion and keeping the input as a number will always fail. Therefore your suggestion and our incorrect casting would still both result in an error.

The provided solution will most of the time work except when there are those edge cases you provided. However, in both cases the casted value will be in the assertion failure message - leading the user to then fix it. For this reason, I say we keep the PR as is and merge.

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: work in progress There is an open PR for this issue [WIP] labels May 18, 2020
@flotwig
Copy link
Contributor

flotwig commented May 18, 2020

However, in both cases the casted value will be in the assertion failure message - leading the user to then fix it.

I don't know, it feels like a big assumption that this will be an improvement to the user experience. I feel like this will be the user story if this change goes in as-is:

  1. User has a 16-digit credit card field on their site and wants to fill it in. (common use case) cy.get('.cc').type('9999999999999999')
  2. User wants to assert that some specially formatted field contains the text '9999999999999999'.
  3. User types cy.get('.cc').should('have.text', 9999999999999999)
  4. 9999999999999999 is over the int limit so it is internally converted to 10000000000000000.
  5. Assertion fails with "Error: expected .cc to contain 9999999999999999 but really contained 10000000000000000"
  6. Now the user is confused... they do one of the following:
    • They open a bug report, stating that "assertions are comparing the wrong numbers", then we point them to this issue and tell them to wrap their expected value in quotes
    • They try changing their test value to a different value, eg 1111111111111111 ( this is NOT over the int limit, so it will work) User goes "huh, weird, why does it work now?" and either opens up a bug report stating "assertions fail if i use 16 9's but succeed if i use 16 1's" or doesn't open up an issue at all and just thinks Cypress is bad at math
    • They happen to have a good knowledge of Javascript and where the integer-float boundary lies and so they intuitively understand that "Error: expected .cc to contain 9999999999999999 but really contained 10000000000000000" can be fixed by setting the expected value as a string, not a number

I think for a significant % of people who run into this issue, the solution will be non-obvious. Compare to the user story if we throw an error up front:

  1. User has a 16-digit credit card field on their site and wants to fill it in. cy.get('.cc').type('9999999999999999')
  2. User wants to assert that some specially formatted field contains the text '9999999999999999'.
  3. User types cy.get('.cc').should('have.text', 9999999999999999)
  4. Assertion fails with "Error: have.text assertions expect a string as a parameter to match against, but a number was passed instead"
  5. User understands that they did not pass a string and changes spec code to expect a string. Test passes.

I get what you mean, users are probably unlikely to run into this bug... but once they do, it will become very confusing very quickly. 🤷

@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels May 19, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 19, 2020

The code for this is done in cypress-io/cypress#7315, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 20, 2020

Released in 4.6.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v4.6.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: unexpected behavior User expected result, but got another
Projects
None yet
3 participants