Add integration tests for collection estimation page #1115

mukeshk · 2019-08-30T07:08:43Z

mystamps-bot · 2019-08-30T07:15:13Z

	1 Warning
⚠️	danger check: pull request description doesn’t contain a link to original issue. Consider adding a comment in the following format: `Addressed to #XXX` where `XXX` is an issue number

Generated by 🚫 Danger

mukeshk · 2019-08-30T07:19:43Z

I will check. the existing test case failed.

php-coder · 2019-08-30T09:16:01Z

Don't forget to also remove related TODO comment (see the original issue).

src/test/robotframework/collection/estimation/logic.robot

src/test/robotframework/selenium.utils.robot

src/test/robotframework/collection/estimation/logic.robot

src/test/robotframework/selenium.utils.robot

src/test/robotframework/collection/estimation/logic.robot

src/main/webapp/WEB-INF/views/collection/estimation.html

src/test/robotframework/collection/estimation/logic.robot

php-coder

See my comments.

mukeshk · 2019-08-31T15:55:37Z

I am done with the comments.

php-coder

Looks good except a couple things that we missed. See my comments.

Also, could you, please, amend the first line of a commit message to be:

test(collection/estimation/logic.robot): add tests for collection estimation page.

src/test/robotframework/collection/estimation/logic.robot

php-coder · 2019-09-01T13:54:47Z

src/test/robotframework/collection/estimation/logic.robot

+	Submit Form                  id=add-series-form
+	Go To                        ${SITE_URL}/collection/paid/estimation
+	Table Cell Should Contain    collection-estimation  2  2  100.00  ${expectedCurrency}
+	Table Cell Should Contain    collection-estimation  3  2  100.00  ${expectedCurrency}


Why we can't use Table Header Should Contain here? I mean, for "Total" it makes sense to use Table Header Should Contain while for the rows we can use Table Cell Should Contain or something similar.

Ok. I thought to remove the dependency on the Table Header. As we wanted to use Table Footer.
I can put it back. Yes, it's more communicative and expressive in terms of Table Header.

php-coder · 2019-09-01T13:55:29Z

src/test/robotframework/collection/estimation/logic.robot

+	Select From List By Value    id=paid-currency  ${expectedCurrency}
+	Submit Form                  id=add-series-form
+	Go To                        ${SITE_URL}/collection/paid/estimation
+	Table Cell Should Contain    collection-estimation  2  2  100.00  ${expectedCurrency}


Here and below -- first argument is locator and we should use id=collection-estimation to make it more obvious for a reader.

It's hard to read what 2 and 2 means here. AFAIR we can use named arguments, like this:

Table Cell Should Contain id=collection-estimation row=2 col=2 text=100.00 ${expectedCurrency}

Could you check whether it works, please?

Err.. forget my comments above. We need a better approach instead of using Table Cell Should Contain.

We need a better approach instead of using Table Cell Should Contain

While it's in a scope of this task, it's out of your budget, so I'm going to think about it later :)

If the table rows have id=series_x and column has series_x_price, where x = <series_id>.
We can have an assertion with xpath - identifying tr[@id=series_1]/td[@id=series_1_price] ... kind of. Just my thought, not sure if that's your concern.

To summarise what needs to be done:

fix "Table Cell Should Contain": add id= to locators, explicitly specify names of arguments (col/row/text)

reduce an indentation between keywords and their parameters in the test cases with "Table Cell Should Contain"

That's it.

Thank you for your patience, Mukesh!

To summarise what needs to be done:

fix "Table Cell Should Contain": add id= to locators, explicitly specify names of arguments (col/row/text)

reduce an indentation between keywords and their parameters in the test cases with "Table Cell Should Contain"

That's it.

Thank you for your patience, Mukesh!

Thanks for your patience as well. :). Let's get it right as per your expecations !

Here and below -- first argument is locator and we should use id=collection-estimation to make it more obvious for a reader.

Next time, when I'll say something wrong, you may quote documentation to me:

Table related keywords, such as Table Should Contain, work differently. By default, when a table locator value is provided, it will search for a table with the specified id attribute.

:-)

php-coder

To summarise what needs to be done:

fix "Table Cell Should Contain": add id= to locators, explicitly specify names of arguments (col/row/text)
reduce an indentation between keywords and their parameters in the test cases with "Table Cell Should Contain"

That's it.

Thank you for your patience, Mukesh!

mukeshk · 2019-09-01T16:55:07Z

php-coder · 2019-09-01T22:08:11Z

In the screenshot I see that you used comma (instead of 2 spaces) to separate arguments. But you've found it already :)

php-coder

LGTM! Thank you! 👍

php-coder · 2019-09-01T22:20:03Z

Whoops, couldn't merge because one item is still here:

Don't forget to also remove related TODO comment (see the original issue).

Let me know when you fix it, so I'll merge.

…imation page. Fix php-coder#893

php-coder · 2019-09-02T09:24:32Z

Excellent! Thank you!

php-coder · 2019-09-02T09:26:55Z

src/test/robotframework/collection/estimation/logic.robot

+	Submit Form                  id=add-series-form
+	Go To                        ${SITE_URL}/collection/paid/estimation
+	Table Cell Should Contain    collection-estimation  row=2  column=2  text=100.00 ${expectedCurrency}
+	# TODO: use "Table Footer Should Contain" instead, when it will be fixed.


Argh, I forgot that we shouldn't use TODO as 0pdd is also looking on it:

ERROR: src/test/robotframework/collection/estimation/logic.robot; puzzle at line #23; TODO found, but puzzle can't be parsed, most probably because TODO is not followed by a puzzle marker, as this page explains: https://github.com/yegor256/pdd#how-to-format

I'll fix it later.

mukeshk requested a review from php-coder as a code owner August 30, 2019 07:08