updated the following keywords so they correctly handle case sensitiv… #76

ractoc · 2019-07-11T09:56:08Z

Conform Issue #75 I have prepared this pull request.

updated the following keywords so they correctly handle case sensitivity conform the Python implementation

Element Should Contain
Element Should Not Contain
Element Text Should Be
Element Text Should Not Be

php-coder · 2019-07-11T10:00:50Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

 	public void elementShouldContain(String locator, String text, String...params) {
-	    String message = robot.getParamsValue(params, 0, "");
+        String message = robot.getParamsValue(params, 0, "");


As we use StringUtils from commons-lang3, perhaps, we can use EMPTY instead of "" here.

php-coder · 2019-07-11T10:02:44Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

-		if (text.equals(actual)) {
-			if (message == null || message.equals("")) {
+		if (textToCompare.equals(actual)) {
+			if (StringUtils.isNotBlank(message)) {


Technically, this looks like a backward incompatible change because isNotBlank isn't the same as isNotEmpty.

I'm fine with using isNotEmpty instead.

php-coder · 2019-07-11T10:11:25Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

+		String textToCompare = text;
+		if (ignoreCase) {
+			actual = actual.toLowerCase();
+			textToCompare = textToCompare.toLowerCase();


String class and StringUtils have methods for case insensitive comparison, perhaps, we could use them. WDYT?

https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#equalsIgnoreCase(java.lang.String)

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#equalsIgnoreCase-java.lang.CharSequence-java.lang.CharSequence-

I kep that toLower from the original code of the class. So I'm fine with any way you want to change that. The main thing that needed adding was the option to chooise between testing with and without case sensitivity.

I'm changing it to:

String actual = elements.get(0).getText(); if (!(ignoreCase && text.equalsIgnoreCase(actual)) || !(!ignoreCase && text.equals(actual))) { }

php-coder · 2019-07-11T10:13:29Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

+		String textToCompare = text;
+		if (ignoreCase) {
+			actual = actual.toLowerCase();
+			textToCompare = textToCompare.toLowerCase();


Using toLowerCase()/toUpperCase() without specifying a locale leads to bugs like that: https://garygregory.wordpress.com/2015/11/03/java-lowercase-conversion-turkey/

:-/

ractoc · 2019-07-11T10:48:02Z

I'll check if I can implement the relevant changes you suggested above later today. More meetings first...

ractoc · 2019-07-11T13:51:23Z

@php-coder I think I got all of the issues sorted.

php-coder · 2019-07-11T14:12:08Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

 		String actual = getText(locator);

-		if (!actual.toLowerCase().contains(text.toLowerCase())) {
+		if ((!(ignoreCase && StringUtils.containsIgnoreCase(actual, text))) || !(!ignoreCase && StringUtils.contains(actual, text))) {


Would it be better for readability if we extract all these logical operations into a variable? Something like this:

boolean contains = ignoreCase ? StringUtils.containsIgnoreCase(actual, text) : StringUtils.contains(actual, text);

if we can't use ternary operation and you think that it's not readable, here is the option with simple if:

boolean contains = false; if (ignoreCase) { contains = StringUtils.containsIgnoreCase(actual, text); } else { contains = StringUtils.contains(actual, text); }

WDYT?

actually, I think the terniary operation should work.
Give me 15 minutes...

And pushed...

ractoc · 2019-07-11T14:26:18Z

I'm heading home now, I'll look into it more tomorrow if there are more findings

php-coder · 2019-07-11T14:57:51Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

 		String actual = getText(locator);

-		if (actual.toLowerCase().contains(text.toLowerCase())) {
+		if (!textContains(actual, text, ignoreCase)) {


Looks like copy&paste mistake -- textContains() should be here.

php-coder · 2019-07-11T14:59:52Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

@@ -1096,4 +1103,16 @@ public static String escapeXpathValue(String value) {
 		return String.format("'%s'", value);
 	}

+	private boolean textContains(String actual, String text, boolean ignoreCase) {


That is even better 👍

One tiny idea: how about adding static modifier to these utility methods?

Hi-Fi · 2019-07-11T17:12:17Z

First of all thank you for the PR.

I think getText can return null, so there might be extra NPEs coming from (original) code, too. If those could be checked at the same time it would be nice. I think most of those would be bypassed by using StringUtils -methods instead of String's own ones.

ractoc · 2019-07-12T06:54:39Z

I did a quick check of the StringUtils methods. If either actual or text is null, those methods return false. So that should work nicely

php-coder · 2019-07-12T08:41:03Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

 		return ignoreCase
 				? StringUtils.containsIgnoreCase(actual, text)
 				: StringUtils.contains(actual, text);
 	}

-	private boolean textIs(String actual, String text, boolean ignoreCase) {
+    public static boolean textIs(String actual, String text, boolean ignoreCase) {


It seems like indentation here is incorrect.

I fixed te indentation. For some reason, this one was using tabs instead of spaces. the other indentations all worked with tabs...

php-coder · 2019-07-12T08:41:49Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

@@ -1103,13 +1103,13 @@ public static String escapeXpathValue(String value) {
 		return String.format("'%s'", value);
 	}

-	private boolean textContains(String actual, String text, boolean ignoreCase) {
+	public static boolean textContains(String actual, String text, boolean ignoreCase) {


I personally don't see a reason to declare these methods as public but it shouldn't hurt.

without making them public, making them static doesn't really make a lot of sense, since they are never called from a static method within the class itself

Yes, I agree that there is no "a lot of sense" and this topic is... debatable. From my point of view, "private static" is still make sense because

it emphasizes the fact that this method doesn't depend on other class members (doesn't use internal state)

JVM doesn't pass this as the first parameter to such the methods

Here are my points. The last call, of course, in on @Hi-Fi and you :)

I'm doing one more commit. Do I leave this in as public static, or revert it back to private?
private static doesn't make much sense since in the class it is never called from a static context.

For me both are ok. Maybe original is better if there's no clear reason for change.

ractoc · 2019-07-12T09:54:07Z

final commit, I hope...

php-coder · 2019-07-12T10:36:26Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Element.java

@@ -25,1075 +26,1093 @@
 @RobotKeywords
 public class Element extends RunOnFailureKeywordsAdapter {

-	/**


:-( That's a little unexpected that now nearly all the lines were modified..

ah, now I see
yes, I converted tabs to spaces, thinking my line was the only line with this problem.
Since this is just about the only change in this commit, it shouldn't be too bad though

php-coder · 2019-07-12T10:45:42Z

@ractoc Thank you, Mark!

If you aren't fully tired with that, you might want to squash 5 commits into single one (see https://davidwalsh.name/squash-commits-git).

…tShouldBe and elementText ShouldNotBe to support either case sensitive or case insensitive operation. This is in line with recent changes to the Python version of the selenium library. Unfortunately, along the way, the indentation has been changed from tabs to all spaces.

ractoc · 2019-07-15T07:01:47Z

That should do it then...
Now, if you set the compare to ignore whitespace changes, you should get only the actual changes

ractoc mentioned this pull request Jul 11, 2019

Element Should (Not) Contain is case sensitive in Python but not in Java #75

Closed

php-coder reviewed Jul 11, 2019

View reviewed changes

php-coder reviewed Jul 12, 2019

View reviewed changes

Hi-Fi merged commit 637fb35 into MarketSquare:develop Jul 16, 2019

updated the following keywords so they correctly handle case sensitiv… #76

updated the following keywords so they correctly handle case sensitiv… #76

Uh oh!

Conversation

ractoc commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ractoc commented Jul 11, 2019

Uh oh!

ractoc commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ractoc commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hi-Fi commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ractoc commented Jul 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ractoc commented Jul 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Jul 12, 2019

Uh oh!

ractoc commented Jul 15, 2019

Uh oh!

Uh oh!

Hi-Fi commented Jul 11, 2019 •

edited

Loading

php-coder Jul 12, 2019 •

edited

Loading