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

Enable object as parameter for css, prop, and attr assertions #7356

Closed
wants to merge 18 commits into from

Conversation

panzarino
Copy link
Contributor

@panzarino panzarino commented May 14, 2020

User facing changelog

Now have the option to pass an object of values to the attr, prop, and css assertion methods.

Additional details

This change was needed because previously passing an object would always make the assertion pass. An exception could be thrown, but actually being able to pass an object would be a better option.

How has the user experience changed?

Take the following test for example:

this.$div = $('<div style=\'display: none; position: absolute;\'>div</div>')

// Group 1 - should pass
expect(this.$div).to.have.css({ display: 'none' })
expect(this.$div).to.have.css({ position: 'absolute' })
expect(this.$div).to.have.css({ display: 'none', position: 'absolute' })

// Group 2 - should not pass
expect(this.$div).to.have.css({ display: 'inline' })
expect(this.$div).to.have.css({ position: 'fixed' })
expect(this.$div).to.have.css({ display: 'inline', position: 'fixed' })
expect(this.$div).to.have.css({ foo: 'bar' })

// Group 3 - should pass
expect(this.$div).not.to.have.css({ display: 'inline' })
expect(this.$div).not.to.have.css({ position: 'fixed' })
expect(this.$div).not.to.have.css({ display: 'inline', position: 'fixed' })
expect(this.$div).not.to.have.css({ foo: 'bar' })

The intended behavior is that groups 1 and 3 pass, while group 2 does not.

Before these changes, groups 1 and 2 would pass while group 3 would not. The screenshot also shows that the correct comparison was not being done:
Screen Shot 2020-05-14 at 8 36 45 PM

After these changes, the correct comparisons are done and group 1 passes while group 2 does not. The screenshot shows how the correct comparison is used:
Screen Shot 2020-05-14 at 8 38 41 PM

As intended, groups 1 and 3 both pass with the correct comparisons being done:
Screen Shot 2020-05-14 at 8 40 12 PM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

Docs changes: cypress-io/cypress-documentation#2803

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 14, 2020

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

  • Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
  • Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

  • The feature/bugfix is self-documenting from within the product.
  • The change provides the end user with a way to fix their problem (no dead ends).

Functionality

  • The code works and performs its intended function with the correct logic.
  • Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
  • The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

  • The code is readable (too many nested 'if's are a bad sign).
  • Names used for variables, methods, etc, clearly describe their function.
  • The code is easy to understood and there are relevant comments explaining.
  • New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
  • There are comments containing link(s) to the addressed issue (in tests and code).

Quality

  • The change does not reimplement code.
  • There's not a module from the ecosystem that should be used instead.
  • There is no redundant or duplicate code.
  • There are no irrelevant comments left in the code.
  • Tests are testing the code’s intended functionality in the best way possible.

Internal

  • The original issue has been tagged with a release in ZenHub.

@cypress
Copy link

cypress bot commented May 14, 2020



Test summary

7555 82 119 17


Run details

Project cypress
Status Failed
Commit 30e2643
Started Jun 16, 2020 10:10 PM
Ended Jun 16, 2020 10:16 PM
Duration 06:35 💡
OS Linux Debian - 10.1
Browser Multiple

View run in Cypress Dashboard ➡️


Failures

Run group: 5x-driver-firefox (Linux, Firefox )
commands/traversals_spec.js Failed
1 src/cy/commands/traversals > should('not.exist')
commands/querying_spec.js Failed
1 ... > waits until button exists
2 ... > waits until button does not exist
3 ... > retries until cannot find element
4 ... > retries until element is invisible
5 ... > retries until element is visible
commands/actions/type_spec.js Failed
1 ... > element
commands/assertions_spec.js Failed
1 ... > works with regular objects
2 ... > resolves eventually not exist
3 ... > no prop, with prop, negation, and chainable
This comment includes only the first 10 test failures. See all 82 failures in the Cypress Dashboard.
Run group: 5x-driver-chrome (Linux, Chrome )
commands/querying_spec.js Failed
1 ... > waits until button exists
2 ... > waits until button does not exist
3 ... > retries until cannot find element
4 ... > retries until element is invisible
5 ... > retries until element is visible
commands/traversals_spec.js Failed
1 src/cy/commands/traversals > should('not.exist')
commands/connectors_spec.js Failed
1 ... > function property > works with numerical indexes
2 ... > accepts a options argument > works with numerical indexes
3 ... > retries until property does NOT exist with an assertion
commands/assertions_spec.js Failed
1 ... > works with regular objects
This comment includes only the first 10 test failures. See all 82 failures in the Cypress Dashboard.
Run group: 2x-desktop-gui (Linux, Electron )
runs_list_spec.js Failed
1 ... > enables refresh button
2 ... > enables refresh button
setup_project_modal_spec.js Failed
1 ... > displays loading view before orgs load
settings_spec.js Failed
1 ... > displays config section
2 ... > displays updated config
3 ... > notes that cypress.json is disabled
4 ... > notes that a custom config is in use
5 ... > when record key loads > displays first Record Key
6 Settings > errors > displays errors
7 ... > displays available editors with preferred one selected
This comment includes only the first 10 test failures. See all 82 failures in the Cypress Dashboard.
Run group: reporter (Linux, Electron )
test_errors_spec.js Failed
1 test errors > code frames > shows code frame when included on error
controls_spec.ts Failed
1 ... > shows 'Tests'
This comment includes only the first 10 test failures. See all 82 failures in the Cypress Dashboard.

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

@panzarino panzarino marked this pull request as ready for review May 15, 2020 00:42
@panzarino panzarino requested a review from kuceb May 15, 2020 00:43
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

1. Assertion display differences

The display of the assertions differs when you use Cypress .should() assertions versus the standard expect. When using .should() it always only displays the last assertion in the Command Log. You should be able to write tests around the number/value of logs displayed in this situation.

My code against <div style="display: block; position: absolute;">div</div>

it('visit', () => {
  cy.visit('index.html')
  cy.get('div').then(($div) => {
    expect($div).to.have.css({ display: 'block' })
    expect($div).to.have.css({ position: 'absolute' })
    expect($div).to.have.css({ display: 'block', position: 'absolute' })
  })
})

it('visit', () => {
  cy.visit('index.html')
  cy.get('div').should('have.css', { display: 'block' })
  cy.get('div').should('have.css', { position: 'absolute' })
  cy.get('div').should('have.css', { display: 'block', position: 'absolute' })
})

Screen Shot 2020-05-20 at 12 36 54 PM

2. Empty object behavior

The assertions always pass when passed an empty Object. I dunno, maybe this is ok? Seems a bit weird though. The assertions don't display in the UI at all, so it seems like a noop.

it('visit', () => {
  cy.visit('index.html')
  cy.get('div').then(($div) => {
    expect($div).to.have.css({})
  })
})

it('visit', () => {
  cy.visit('index.html')
  cy.get('div').should('have.css', {})
})

Screen Shot 2020-05-20 at 12 41 42 PM

cli/types/index.d.ts Outdated Show resolved Hide resolved
@panzarino
Copy link
Contributor Author

@chrisbreiding and I decided that its probably best to not make these changes as they are fairly inconsistent with other methods and the existing Chai api, plus there would have to be a unique workaround to fix the logging issues. For now, I'm just going to submit another PR that causes a test to fail when passed an object rather than blindly succeed.

@panzarino panzarino closed this Jun 2, 2020
@brian-mann
Copy link
Member

I am not convinced this should not be implemented... I don't believe there are logging issues with this.

@jennifer-shehane
Copy link
Member

Perhaps this illustrates the issue more clearly. When using .should(), only the last assertion in the object is logged as opposed to logging all assertions within a normal expect.

@brian-mann Can you explain more clearly why you consider this not an issue?

Screen Shot 2020-06-12 at 11 27 49 AM

Also, my second point about asserting on an empty object hasn't been responded to - whether this is desired behavior or not.

@brian-mann
Copy link
Member

brian-mann commented Jun 12, 2020

With the object literal pattern - its really a "subset" kind of assertion, which means that the key's of the object are what you're asserting against - not the entirety of the derived CSS. An element may have 100 inherited styles, and the keys in the object are the things you're validating.

Therefore, when given a blank object then I believe this would give you a specific error telling you to provide some kind of assertion otherwise there's nothing specifically we can assert about.

panzarino and others added 3 commits June 16, 2020 15:38
… a command and the previous command in a hook was a cy.then
- capture the assertionId and pass it around as state
- if the same underlying assertion is generating multiple chai
assertions, then merge the new assertions on the existing log instead
of generating multiple command logs
@jennifer-shehane
Copy link
Member

Closing due to inactivity. We can reopen a new PR if we feel the work is something we want to prioritize in the future.

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.

Passing an object to 'css', 'prop', or 'attr' assertions always passes
4 participants