cy.contains() improvements: normalizing whitespaces, case-sensi… #5653

sainthkh · 2019-11-11T08:58:51Z

These 2 are fixed together because the implementation for one can affect the other.

User facing changelog

When there are multiple white spaces, it is replaced to a single white space except in <pre> tag.
Added matchCase option to cy.contains().

Additional details

Why was this change necessary?

issue #92

Sometimes when I issue .contains() commands with text I expect to be found, it doesn't match my element as expected due to extra whitespace/new lines in my element.

HTML
<h1>hello
world</h1>
Test
cy.contains("hello world") 
// CypressError: Timed out retrying: Expected to find content: 'hello world' but never did.

issue #2785

We can currently use regex, but in some cases, we need Cypress's out-of-the-box option.

What is affected by this change?

cy.contains() can do more things like ignoring multiple spaces and case-sensitivity.

Any implementation details to explain?

A new contains implementation has been added.

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has a PR for user-facing changes been opened in cypress-documentation? => Cypress issue#92: cy.contains() doc. cypress-documentation#2397
Have API changes been updated in the type definitions?~~
~~Have new configuration options been added to the cypress.schema.json?~~

Documentation and type definition tasks will be started when this PR is ready and reviewed.

cypress-bot · 2019-11-11T08:58:54Z

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.

chrisbreiding · 2020-01-06T14:49:34Z

Is it possible this is a breaking change? Could a user have tests that rely on the old behavior and they will fail once this is introduced? Just need to know so that we can hold off on releasing this until the next major version of Cypress.

sainthkh · 2020-01-07T00:44:08Z

@chrisbreiding #2785 doesn't break anything but #92 can be a breaking change. Then, should I change the target branch to v4.0-release?

jennifer-shehane · 2020-01-07T07:17:56Z

@sainthkh Yes, breaking changes should target v4.0-release release

sainthkh · 2020-01-07T07:56:30Z

Rebased and tests all passed.

chrisbreiding · 2020-01-14T18:44:27Z

I think the only thing left for this is to update the documentation. @sainthkh Can you open a PR targeting cypress-io/cypress-documentation#1710? Please update the cy.contains() page to detail this new behavior and update the 4.0 migration guide with an example of a test that will break with this change and how to fix it.

sainthkh · 2020-01-15T01:32:49Z

Added option type for cy.contains().
Wrote documentation at Cypress issue#92: cy.contains() doc. cypress-documentation#2397
Added a new test for leading/trailing whitespaces.

jennifer-shehane · 2020-01-15T07:10:34Z

packages/driver/src/cy/commands/querying.js

+      const normalizeWhitespaces = (elem) => {
+        let testText = elem.textContent || elem.innerText || $.text(elem)
+
+        if (elem.tagName === 'PRE') {


why the exception for pre tags? I mean, I can kind of get the logic, but it does seem strange to make an exception for a single tag and this not be documented anywhere. I guess that's my concern. Someone opening an issue one day like, 'this is working differently here' and we have this undocumented exception.

pre tag preserves whitespaces like this:

https://codepen.io/sainthkh/pen/jOEpzmX

So, it is the exception. Because I thought it reflects how browsers work.

Do you think we need a section for this behavior in contains() documentation?

Yeah...probably adding a section in the Notes of the .contains() doc so that people can find this somehow. There's are other exception notes in there.

What about elements styled with white-space: pre;?
We have cases where upgrading to Cypress 4 breaks tests where we're explicitily want to vaildate that the whitespace is present as it is rendered. Should there be a flag allowing override of this behaviour?

@mariusnn It's going on #6411.

jennifer-shehane

I ran our Dashboard tests through this branch since we have about 1,000 uses of .contains() in there. I didn't see anything really strange pop up. It doesn't even seem to be breaking any of our usecases - probably because we've avoided asserting on newlines before.
I functionally tested this on a few examples and it seems to be working as intended.

It feels a bit weird that this test case passes...

// <h1>hello world</h1>
cy.contains(/Hello World/i, { matchCase: true })

Also the exception of the <pre> tag seems a bit weird as undocumented behavior as mentioned in another comment.

sainthkh · 2020-01-15T07:24:28Z

// <h1>hello world</h1>
cy.contains(/Hello World/i, { matchCase: true })

In this case, how about throwing an error?:

cy.contains() option `matchCase` and regex flag conflicts.

sainthkh · 2020-01-16T03:32:50Z

Now, an error is thrown for cy.contains(/Hello World/i, { matchCase: true })

packages/driver/src/cy/commands/querying.js

sainthkh · 2020-01-28T02:00:52Z

Fixed how error message is thrown. But it fails a lot because of "Error: Resolution method is overspecified. Specify a callback or return a Promise; not both."

It was introduced in bcc48f2. Needs to find way to fix it.

Opened an issue at #6260.

jennifer-shehane · 2020-01-29T15:40:17Z

Yah, the 4.0 branch is broken atm, so trying to fix that up to get these tests passing.

sainthkh force-pushed the issue-92-fix branch 2 times, most recently from 2222f86 to 3caf09b Compare December 3, 2019 03:58

sainthkh marked this pull request as ready for review December 3, 2019 03:59

sainthkh changed the title ~~[wip] cy.contains() improvements: normalizing whitespaces, case-sensitivity. (Waiting for #5643)~~ cy.contains() improvements: normalizing whitespaces, case-sensitivity. Dec 3, 2019

sainthkh changed the title ~~cy.contains() improvements: normalizing whitespaces, case-sensitivity.~~ wip cy.contains() improvements: normalizing whitespaces, case-sensitivity. Dec 3, 2019

sainthkh changed the title ~~wip cy.contains() improvements: normalizing whitespaces, case-sensitivity.~~ cy.contains() improvements: normalizing whitespaces, case-sensitivity. Dec 3, 2019

jennifer-shehane requested a review from a team December 19, 2019 06:37

jennifer-shehane added the type: breaking change Requires a new major release version label Jan 7, 2020

sainthkh changed the base branch from develop to v4.0-release January 7, 2020 07:20

sainthkh force-pushed the issue-92-fix branch from 6b66256 to f5a986a Compare January 7, 2020 07:28

sainthkh mentioned this pull request Jan 15, 2020

Cypress issue#92: cy.contains() doc. cypress-io/cypress-documentation#2397

Merged

1 task

jennifer-shehane requested review from jennifer-shehane and chrisbreiding and removed request for a team January 15, 2020 06:23

jennifer-shehane reviewed Jan 15, 2020

View reviewed changes

bahmutov added 2 commits January 22, 2020 14:10

migrate vscode eslint settings

105b13c

merged develop

bcc48f2

chrisbreiding requested changes Jan 27, 2020

View reviewed changes

packages/driver/src/cy/commands/querying.js Outdated Show resolved Hide resolved

sainthkh added 2 commits January 28, 2020 10:12

Handles whitespaces with newlines.

bd66f35

Feature: contains() matches case insensitivity

7408ec3

sainthkh added 8 commits January 28, 2020 10:12

Fixed merge conflicts.

5acb51b

Fixed conflicts.

b31a7dd

Added option type, CaseMatchable.

12c79f0

Fixed lint error.

53b3365

Added test for leading/trailing spaces.

fc2c1d0

Add an error message for regex and matchCase conflict.

ff5b9c5

Fix the valid case that throws an error.

89cc3f8

Fix how error message is thrown.

ec02a09

sainthkh force-pushed the issue-92-fix branch from 1b53324 to ec02a09 Compare January 28, 2020 01:50

jennifer-shehane added 2 commits January 29, 2020 14:21

update some cli deps that have fallen out of date since last commit

6e28b78

Merge branch 'v4.0-release' into issue-92-fix

67aabe9

jennifer-shehane requested a review from chrisbreiding January 29, 2020 15:35

chrisbreiding force-pushed the v4.0-release branch from 6e28b78 to a0eddc2 Compare January 29, 2020 17:19

chrisbreiding added 2 commits January 29, 2020 15:33

Merge branch 'v4.0-release' into issue-92-fix-local

8f2cef3

update cli snapshot

4a4150c

chrisbreiding approved these changes Jan 29, 2020

View reviewed changes

chrisbreiding changed the title ~~cy.contains() improvements: normalizing whitespaces, case-sensitivity.~~ cy.contains() improvements: normalizing whitespaces, case-sensi… Jan 30, 2020

chrisbreiding merged commit 7ff91ed into cypress-io:v4.0-release Jan 30, 2020

This was referenced Jan 30, 2020

Have .contains() strip new lines of targets. #92

Closed

Introduce non case sensitive matching for cy.contains() #2785

Closed

jennifer-shehane mentioned this pull request Feb 10, 2020

cy.contains() falsely failing when using negative assertion 'not.contain' #6384

Closed

c32hedge mentioned this pull request Feb 10, 2020

Allow cy.contains() to opt out of removing whitespace matching #6405

Open

kuceb mentioned this pull request Feb 12, 2020

fix pollution of :contains pseudo selector #6434

Merged

2 tasks

katie-cawthorne mentioned this pull request Nov 18, 2020

💄 PIC-917: Added whitespace formatting: ministryofjustice/prepare-a-case#228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cy.contains() improvements: normalizing whitespaces, case-sensi… #5653

cy.contains() improvements: normalizing whitespaces, case-sensi… #5653

sainthkh commented Nov 11, 2019 •

edited

Loading

cypress-bot bot commented Nov 11, 2019

chrisbreiding commented Jan 6, 2020

sainthkh commented Jan 7, 2020

jennifer-shehane commented Jan 7, 2020

sainthkh commented Jan 7, 2020

chrisbreiding commented Jan 14, 2020

sainthkh commented Jan 15, 2020

jennifer-shehane Jan 15, 2020

sainthkh Jan 15, 2020 •

edited

Loading

jennifer-shehane Jan 15, 2020

mariusnn Mar 17, 2020

sainthkh Mar 18, 2020

jennifer-shehane left a comment •

edited

Loading

sainthkh commented Jan 15, 2020 •

edited

Loading

sainthkh commented Jan 16, 2020 •

edited

Loading

sainthkh commented Jan 28, 2020 •

edited

Loading

jennifer-shehane commented Jan 29, 2020

cy.contains() improvements: normalizing whitespaces, case-sensi… #5653

cy.contains() improvements: normalizing whitespaces, case-sensi… #5653

Conversation

sainthkh commented Nov 11, 2019 • edited Loading

User facing changelog

Additional details

Why was this change necessary?

issue #92

issue #2785

What is affected by this change?

Any implementation details to explain?

How has the user experience changed?

PR Tasks

cypress-bot bot commented Nov 11, 2019

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

chrisbreiding commented Jan 6, 2020

sainthkh commented Jan 7, 2020

jennifer-shehane commented Jan 7, 2020

sainthkh commented Jan 7, 2020

chrisbreiding commented Jan 14, 2020

sainthkh commented Jan 15, 2020

jennifer-shehane Jan 15, 2020

Choose a reason for hiding this comment

sainthkh Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

jennifer-shehane Jan 15, 2020

Choose a reason for hiding this comment

mariusnn Mar 17, 2020

Choose a reason for hiding this comment

sainthkh Mar 18, 2020

Choose a reason for hiding this comment

jennifer-shehane left a comment • edited Loading

Choose a reason for hiding this comment

sainthkh commented Jan 15, 2020 • edited Loading

sainthkh commented Jan 16, 2020 • edited Loading

sainthkh commented Jan 28, 2020 • edited Loading

jennifer-shehane commented Jan 29, 2020

sainthkh commented Nov 11, 2019 •

edited

Loading

sainthkh Jan 15, 2020 •

edited

Loading

jennifer-shehane left a comment •

edited

Loading

sainthkh commented Jan 15, 2020 •

edited

Loading

sainthkh commented Jan 16, 2020 •

edited

Loading

sainthkh commented Jan 28, 2020 •

edited

Loading