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

Method $.shouldHave(text) should verify the whole text (not a substring) #1780

Closed
asolntsev opened this issue Apr 21, 2022 · 11 comments · Fixed by #1783
Closed

Method $.shouldHave(text) should verify the whole text (not a substring) #1780

asolntsev opened this issue Apr 21, 2022 · 11 comments · Fixed by #1783
Assignees
Milestone

Comments

@asolntsev
Copy link
Member

asolntsev commented Apr 21, 2022

The problem

Current behaviour of $.shouldHave(text()) is misleading. People might think it verifies the whole text, but it actually verifies only a substring.

For example

The test: $.shouldHave(text("error")); may be green even if html has text <div>NO errors</div> which has the opposite meaning. This behaviour can cause false positive tests.

Historical reasons

Many years ago I decided to check a substring in order to ignore spaces, tabs, newlines etc. Now it seems it was not really needed because the condition text already implements this logic inside.

For backward compatibility

I suggest to

  1. change the default behaviour of $.shouldHave(text()) to verify the whole text
  2. introduce one more setting like Configuration.textCheck = FULL_TEXT | PARTIAL_TEXT; which allows users to restore the previous behaviour in case if they get a lot of failing tests.

Tell us about your environment

  • Selenide Version: 6.4.0
@asolntsev asolntsev self-assigned this Apr 21, 2022
@asolntsev asolntsev added this to the 6.5.0 milestone Apr 21, 2022
asolntsev added a commit that referenced this issue Apr 26, 2022
asolntsev added a commit that referenced this issue Apr 26, 2022
asolntsev added a commit that referenced this issue Apr 26, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Apr 27, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Apr 27, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Apr 27, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Apr 30, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Apr 30, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
@plagov
Copy link
Contributor

plagov commented May 4, 2022

In the beginning, one might indeed think that text() would check the whole exact text of the element.

Although, the option with a configuration is backwards compatible, having yet another configuration is not the best solution, IMO.

If the decision would be to switch the text() to a full-text match, then we definitely need something like a partialText() condition for a substring matching.

@asolntsev
Copy link
Member Author

@plagov Yes, PR #1783 introduces partialText().

@cocorossello
Copy link
Contributor

I would go with partialText() and exactText() and deprecate text(). Maybe those should also have an optional flag to make it case sensitive?

Changing text() to check the exact text is a big breaking change, in my opinion.

@asolntsev asolntsev modified the milestones: 6.5.0, 6.6.0 May 17, 2022
@asolntsev asolntsev removed this from the 6.6.0 milestone Jun 6, 2022
asolntsev added a commit that referenced this issue Jun 26, 2022
# Conflicts:
#	src/main/java/com/codeborne/selenide/conditions/Text.java
#	statics/src/test/java/integration/ElementTextTest.java
#	statics/src/test/java/integration/SelectsTest.java
asolntsev added a commit that referenced this issue Jun 26, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Jun 26, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Jun 26, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Jun 26, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Aug 3, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Aug 3, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Aug 3, 2022
asolntsev added a commit that referenced this issue Aug 3, 2022
* "#text-area" is automatically filled by JS at the same time
* it's better to use another text area which is always empty.
asolntsev added a commit that referenced this issue Aug 3, 2022
* "#text-area" is automatically filled by JS at the same time
* it's better to use another text area which is always empty.
asolntsev added a commit that referenced this issue Aug 3, 2022
... to keep backward compatibility. Let's change it to FULL_TEXT in a next major release 7.0.0
@asolntsev asolntsev added this to the 6.7.0 milestone Aug 3, 2022
@asolntsev
Copy link
Member Author

@cocorossello Sure, it's a breaking change. I suggest to make it configurable:

  Configuration.textCheck = PARTIAL_TEXT;   // current default
  Configuration.textCheck = FULL_TEXT;         // the "new way". 

We could make FULL_TEXT default in next major release 7.0.0

asolntsev added a commit that referenced this issue Aug 3, 2022
+ fix existing integration tests that assumed that `$.shouldHave(text)` checks a substring
asolntsev added a commit that referenced this issue Aug 3, 2022
* the text might be "Item #1" or "Updated Item #1" depending on browser speed
* the full text should be "x (1)" meaning "1 change". And it seems to work stabely in all browsers.
asolntsev added a commit that referenced this issue Aug 3, 2022
* "#text-area" is automatically filled by JS at the same time
* it's better to use another text area which is always empty.
asolntsev added a commit that referenced this issue Aug 3, 2022
... to keep backward compatibility. Let's change it to FULL_TEXT in a next major release 7.0.0
@plagov
Copy link
Contributor

plagov commented Aug 5, 2022

@asolntsev do you have any plans or thoughts on the exactText() assertion once the text() one will check for the full text by default after some time? Will it be deprecated in favour of the text() or left as an alias/synonym?

@asolntsev
Copy link
Member Author

@plagov Yes, when Configuration.textCheck = FULL_TEXT then exactText() is synonym for just text().
May be we can mark them as deprecated in some future versions of Selenide. We'll see.

@fabb
Copy link

fabb commented Nov 10, 2023

We could make FULL_TEXT default in next major release 7.0.0

@asolntsev has this happened? i don't see it in the 7.0 release notes.

@fabb
Copy link

fabb commented Nov 10, 2023

Is there a partial variant of CollectionCondition#itemWithText?

@asolntsev
Copy link
Member Author

Hm... Good point!

But no, FULL-TEXT was not made the default in Selenide 7.0.0

I am still afraid that most users hasn't enabled this flag, and this change would trigger too big hassle.. What do you think?

@fabb
Copy link

fabb commented Nov 10, 2023

hm, we had to migrate a few things anyways in the 7.0 major update (e.g. WebElementCondition or ElementsCollection no longer being a collection subclass), so i guess it would not be too much hassle. but i guess it should be done in a way users really see it as compile error or at least as a warning, e.g. as @cocorossello suggested:

I would go with partialText() and exactText() and deprecate text(). Maybe those should also have an optional flag to make it case sensitive?

Changing text() to check the exact text is a big breaking change, in my opinion.

@asolntsev
Copy link
Member Author

To be honest, I forgot about FULL_TEXT when released 7.0.0.
I looked at all "deprecated" words in the project, but FULL_TEXT was not "deprecated". :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants