Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters #41

subiron · 2017-01-03T14:53:24Z

-Click and Hide modifiers allows to use css selectors as parameters (for now, only xpath was an option)
-Added ReplaceText modifier and it's tests

I hereby agree to the terms of the AET Contributor License Agreement.

Update Hide Modifier with find elements by css selector #15

…attributes

malaskowski

Can you please also update click and hide tests so that css selector is checked there?

Great stuff 👍

malaskowski · 2017-01-04T08:01:03Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+            if (StringUtils.isNumeric(timeoutString)) {
+                timeoutInSeconds = TimeUnit.SECONDS.convert(Long.valueOf(timeoutString), TimeUnit.MILLISECONDS);
+                if (timeoutInSeconds < 0) {
+                    throw new ParametersException("'timeout' parameter value on Click Modifier should be greater or equal zero.");


Please change this error message - it looks that WebElementsLocatorParams are used not only in Click Modifier.

Maybe it would be good idea to check 0 < timeoutInSeconds <= 15 in a single condition and serve common validation message: 'Timeout' parameter value should be in range 0 - 15 seconds or sth. like that.

malaskowski · 2017-01-04T08:07:25Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/hide/HideModifier.java


-  public CollectorStepResult hideElement(WebDriver driver, String xpath) throws ProcessingException {
+  public CollectorStepResult hideElement(By locator) throws ProcessingException {


It looks that hide does not support timeout value.

malaskowski · 2017-01-04T08:12:54Z

...bs/src/main/java/com/cognifide/aet/job/common/modifiers/replacetext/ReplaceTextModifier.java

+        value = StringUtils.defaultIfBlank(params.get(VALUE_PARAM), "");
+    }
+
+    public CollectorStepResult replaceText() throws ProcessingException {


No support for timeout - should it be here? Maybe it is unnecessary in WebElementsLocatorParams?

it's now added (including documentation)

malaskowski · 2017-01-04T08:15:09Z

...rc/test/java/com/cognifide/aet/job/common/modifiers/replacetext/ReplaceTextModifierTest.java

+
+import java.util.Map;
+
+import static org.mockito.Mockito.*;


Please don't import static with *.

malaskowski · 2017-01-04T08:17:39Z

documentation/src/main/wiki/ClickModifier.md

@@ -11,7 +11,8 @@ Module name: **click**
 ##### Parameters
 | Parameter | Default value | Description | Mandatory |
 | --------- | ------------- | ----------- | --------- |
-| `xpath` | | xpath of the element to be clicked | yes |
+| `xpath` | | xpath of the element to be clicked        | yes |


Please mention that only one selector may be used in each click modifier entry (maybe it is wroth to mention the obvious, that there may be some selectors with xpath and some with css in the same test).

subiron · 2017-01-04T09:08:26Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

@@ -0,0 +1,88 @@
+/**


I would be grateful for any suggestion for better place for this class
(package/name)

I guess this is a very good place for this class.

kwatral · 2017-01-05T07:32:29Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+
+    protected long getTimeoutInSeconds() {
+        return timeoutInSeconds;
+


kwatral · 2017-01-05T07:35:44Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+        xpath = params.get(XPATH_PARAM);
+        css = params.get(CSS_PARAM);
+
+        if (StringUtils.isNotBlank(xpath) == StringUtils.isNotBlank(css)) {


is this condition ok? what about situation when both, xpath and css are set? logical OR in the exception message means that both can be set. Either fix the condition or fix the exception message

kwatral · 2017-01-05T07:38:16Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+                if (timeoutInSeconds < 0) {
+                    throw new ParametersException("'timeout' parameter value should be greater or equal zero.");
+                } else if (TIMEOUT_SECONDS_MAX_VALUE < timeoutInSeconds) {
+                    throw new ParametersException("'timeout' parameter value can't be greater than 15 seconds.");


replace the number from message by the constant TIMEOUT_SECONDS_MAX_VALUE

kwatral · 2017-01-05T07:38:59Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+    }
+
+    protected String getSelectorType() {
+        return (xpath != null) ? XPATH_PARAM : CSS_PARAM;


remove brackets

kwatral · 2017-01-05T07:43:39Z

...bs/src/main/java/com/cognifide/aet/job/common/modifiers/replacetext/ReplaceTextModifier.java

+            By elementLocator = getLocator();
+            waitForElementToBePresent(webDriver, elementLocator);
+
+            List<WebElement> webElements = webDriver.findElements(elementLocator);


lines 81-84 can be moved to the parent class as a method

Maybe we could separate finding an element and updating text into two separate methods with separate try-catch blocks? (nice to have)

kwatral · 2017-01-05T07:45:25Z

...bs/src/main/java/com/cognifide/aet/job/common/modifiers/replacetext/ReplaceTextModifier.java

+    private static final Logger LOG = LoggerFactory.getLogger(ReplaceTextModifier.class);
+
+    private static final String ATTRIBUTE_PARAM = "attributeName";
+    private static final String VALUE_PARAM = "value";


IMO, you missed the gap

wiiitek · 2017-01-16T21:55:12Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+    protected void setElementParams(Map<String, String> params) throws ParametersException {
+        xpath = params.get(XPATH_PARAM);
+        css = params.get(CSS_PARAM);
+        if (StringUtils.isBlank(xpath) == StringUtils.isBlank(css)) {


if both parameters are provided, could we have better exception message? (should have)

imo "Either" should be sufficient in this case

wiiitek · 2017-01-16T21:58:07Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+        wait.until(ExpectedConditions.presenceOfElementLocated(elementLocator));
+    }
+
+    private void setTimeOutParam(String timeoutString) throws ParametersException {


what do you think about renaming this method so that only simple setters has the name using set... (nice to have)

any suggestion?

initializeTimeOutParam?

wiiitek

Thank you @subiron for your work!
This is nice piece of code. I have added some comments though.

wiiitek · 2017-01-16T22:01:47Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/WebElementsLocatorParams.java

+    }
+
+    protected By getLocator() {
+        return xpath != null ? By.xpath(xpath) : By.cssSelector(css);


We may have only one field in thi class By by and initialize it in constructor.
Then the getLocator method would be a simple getter (the name suggests that). (nice to have)

wiiitek · 2017-01-16T22:03:26Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/click/ClickModifier.java

      if (elementToClick.isDisplayed()) {
        elementToClick.click();
        result = CollectorStepResult.newModifierResult();
      } else {
-        final String message = String.format("Element defined by xpath: '%s' is not yet visible! {}", xpath);
+        final String message = String.format("Element defined by %s: '%s' is not yet visible!", getSelectorType(), getSelectorValue());


Maybe By class has a nice toString() method and we could use it instead of saving selector type in WebElementsLocatorParams class? (nice to have)

wiiitek · 2017-01-16T22:05:20Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/click/ClickModifier.java

  @Override
  public CollectorStepResult collect() throws ProcessingException {
    CollectorStepResult result;
    try {
-      WebElement elementToClick = getElementByXpath(webDriver, xpath);
+        By elementLocator = getLocator();
+        waitForElementToBePresent(webDriver, elementLocator);


I think we should rather wait for element to be clickable instead of present / visible. (must have)

wiiitek · 2017-01-16T22:07:58Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/hide/HideModifier.java

-import org.openqa.selenium.NoSuchElementException;
-import org.openqa.selenium.WebDriver;
-import org.openqa.selenium.WebElement;
+import org.openqa.selenium.*;


We probably don't want to use asterisk imports.. do we? (should have)

fixed,
but I'm not sure if it's better in any way,
why we're fallow this rule?

wiiitek · 2017-01-16T22:15:16Z

core/jobs/src/main/java/com/cognifide/aet/job/common/modifiers/hide/HideModifier.java

      }
      result = CollectorStepResult.newModifierResult();
-    } catch (NoSuchElementException e) {
-      final String message = String.format("Error while hiding element '%s'. %s", xpath, e.getMessage());
+    } catch (WebDriverException e) {


We could have two methods: one for finding element and the other for hiding it.
This way we could have two separate try-catch block.
(nice to have)

wiiitek · 2017-01-16T22:37:05Z

core/jobs/src/test/java/com/cognifide/aet/job/common/modifiers/hide/HideModifierTest.java

+
+  @Test(expected = ParametersException.class)
+  public void setParameters_CssAndXpathAreNotPassed_ValidationPassedUnsuccessfuly() throws ParametersException {
+    when(properties.getUrl()).thenReturn(URL);


It looks like this properties mock is ever used. If this is the case it shold be removed.
Then test methods from line 96 and 101 are the same? (must have)

wiiitek · 2017-01-16T22:39:27Z

...rc/test/java/com/cognifide/aet/job/common/modifiers/replacetext/ReplaceTextModifierTest.java

+
+  @Test(expected = ParametersException.class)
+  public void setParameters_CssAndXpathAreNotPassed_ExceptionIsThrown() throws ParametersException {
+    when(properties.getUrl()).thenReturn(URL);


Do we need this properties mock? If not please remove it. If yes, please make it more clear what is the purpose of this mock. (must have).

wiiitek · 2017-01-16T22:42:32Z

documentation/src/main/wiki/Comparators.md

@@ -79,9 +83,12 @@ The configuration above will trigger three screens collections (desktop, tablet
 <collect>
    <open/>
    <sleep duration="1000"/>


Thank you for those changes! They are not obvious and it is very good that we have them in docs!
This configuration looks a little bit redundant, but I believe this is what is required, isn't it?
👍

wiiitek · 2017-01-16T22:53:30Z

documentation/src/main/wiki/releases/2.0.0/Documentation-2.0.0.md

@@ -1916,7 +1918,7 @@ No parameters.

 | ! Important information |
 |:----------------------- |
-| Timeout for waiting is 10000 milliseconds.<br/><br/> Page is checked every 1000 miliseconds. |
+| Timeout for waiting is 10000 milliseconds.<br/><br/> Page is checked every 1000 milliseconds. |


wiiitek · 2017-01-16T22:54:44Z

integration-tests/sample-site/src/main/webapp/sanity/modifiers/replaceText/success.jsp

+<%
+Long millis = System.currentTimeMillis();
+%>
+<p id="demo1">Replace me by Xpath 1: <%=millis%></p>


I like this very much! Nice one! 👍

# Conflicts: # core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/w3chtml5/W3cHtml5ComparatorJob.java

…enium

subiron added 2 commits January 3, 2017 14:06

Update Click Modifier with find elements by css selector #14

68a7df1

Update Hide Modifier with find elements by css selector #15

AETS-166 feature: replacing text inside tags (innerHtml) and in tags …

801a2f0

…attributes

subiron added the enhancement label Jan 3, 2017

subiron self-assigned this Jan 3, 2017

subiron requested review from mchrominski, wiiitek and malaskowski January 3, 2017 14:53

AETS-166 added missing license

1dabb81

malaskowski reviewed Jan 4, 2017

View reviewed changes

ZGCQ-7511 fixed documentation

25dccc3

subiron commented Jan 4, 2017

View reviewed changes

ZGCQ-7511 fixes, cr, typos

9dbb988

kwatral reviewed Jan 5, 2017

View reviewed changes

subiron added 3 commits January 5, 2017 10:18

ZGCQ-7511 cr fixes

3f3b61f

ZGCQ-7511 cr fixes

6b53a5d

ZGCQ-7511 cr fixes

91a725e

wiiitek reviewed Jan 16, 2017

View reviewed changes

wiiitek added 2 commits January 16, 2017 23:49

Update ReplaceTextModifier.md

216d5dd

Update HideModifier.md

3af8f83

wiiitek suggested changes Jan 16, 2017

View reviewed changes

subiron added 5 commits January 19, 2017 11:53

ZGCQ-7582 cr fixes

5d10490

Merge branch 'master' into feature/modifiers_enhancements

941ced8

# Conflicts: # core/jobs/src/main/java/com/cognifide/aet/job/common/comparators/w3chtml5/W3cHtml5ComparatorJob.java

removed one test due to issue with mocking some internal parts of sel…

360801c

…enium

Code reformat

8f353b1

fix after merge

4832103

subiron merged commit cc3512a into master Jan 20, 2017

subiron deleted the feature/modifiers_enhancements branch January 23, 2017 07:31

This was referenced Feb 22, 2017

Update Hide Modifier with find elements by css selector #15

Closed

Update Click Modifier with find elements by css selector #14

Closed

subiron changed the title ~~Feature/modifiers enhancements~~ Added ReplaceText modifier and Modifiers enhancements Feb 24, 2017

subiron changed the title ~~Added ReplaceText modifier and Modifiers enhancements~~ Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters Feb 24, 2017

subiron mentioned this pull request Feb 24, 2017

Update AET version in vagrantfile and update changelog #63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters #41

Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters #41

subiron commented Jan 3, 2017 •

edited

Loading

malaskowski left a comment

malaskowski Jan 4, 2017

malaskowski Jan 4, 2017

subiron Jan 5, 2017

malaskowski Jan 4, 2017

subiron Jan 5, 2017

malaskowski Jan 4, 2017

malaskowski Jan 4, 2017

subiron Jan 4, 2017 •

edited

Loading

malaskowski Jan 4, 2017

kwatral Jan 5, 2017

kwatral Jan 5, 2017

kwatral Jan 5, 2017

kwatral Jan 5, 2017

kwatral Jan 5, 2017

wiiitek Jan 16, 2017

kwatral Jan 5, 2017

wiiitek Jan 16, 2017 •

edited

Loading

subiron Jan 18, 2017 •

edited

Loading

wiiitek Jan 16, 2017

subiron Jan 18, 2017

wiiitek Jan 19, 2017

wiiitek left a comment

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017

subiron Jan 19, 2017 •

edited

Loading

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017 •

edited

Loading

wiiitek Jan 16, 2017

wiiitek Jan 16, 2017


		public CollectorStepResult hideElement(WebDriver driver, String xpath) throws ProcessingException {
		public CollectorStepResult hideElement(By locator) throws ProcessingException {


		protected long getTimeoutInSeconds() {
		return timeoutInSeconds;

Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters #41

Added ReplaceText modifier and Click and Hide modifiers allows to use css selectors as parameters #41

Conversation

subiron commented Jan 3, 2017 • edited Loading

malaskowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subiron Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiiitek Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

subiron Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiiitek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subiron Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiiitek Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subiron commented Jan 3, 2017 •

edited

Loading

subiron Jan 4, 2017 •

edited

Loading

wiiitek Jan 16, 2017 •

edited

Loading

subiron Jan 18, 2017 •

edited

Loading

subiron Jan 19, 2017 •

edited

Loading

wiiitek Jan 16, 2017 •

edited

Loading