Change type to var #6

ufjena · 2021-04-12T19:53:23Z

No description provided.

occupant23

see the first comments

occupant23 · 2021-04-28T10:48:16Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

    };

    @Given("^login page is loaded$")
    @Step("open login page")
    public static LoginPage loginPage()
    {
        // open login page and check for expected page
-        LoginPage loginPage = homepage().userMenu.openLogin();
+        var loginPage = homepage().userMenu.openLogin();
        loginPage.isExpectedPage();


change to return loginPage.isExpectedPage(); so avoid one extra line

occupant23 · 2021-04-28T10:48:36Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

@@ -42,7 +40,7 @@ public static LoginPage loginPage()
    public static RegisterPage registerPage()
    {
        // open login page and check for expected page
-        RegisterPage registerPage = homepage().userMenu.openRegister();
+        var registerPage = homepage().userMenu.openRegister();
        registerPage.isExpectedPage();


change to return loginPage.isExpectedPage(); so avoid one extra line

occupant23 · 2021-04-28T10:50:45Z

src/test/java/posters/cucumber/support/OrderSupport.java

@@ -33,24 +33,24 @@ public OrderSupport(GlobalStorage storage)
    @Given("^new user with \"([^\"]*)\", \"([^\"]*)\", \"([^\"]*)\", \"([^\"]*)\" is registered and logged in$")
    public void registerAndLogIn(String firstName, String lastName, String email, String password)
    {
-        RegisterPage registerPage = OpenPageFlows.registerPage();
+        var registerPage = OpenPageFlows.registerPage();
        registerPage.isExpectedPage();


Please check all "Support" classes. We should have added the isExpectedPage already in the open function for the page. so a second call should not be needed.

occupant23 · 2021-04-28T11:19:36Z

src/test/java/posters/cucumber/support/OrderSupport.java

-        OrderHistoryPage orderHistory = accountOverview.openOrderHistory();
+        var accountOverviewPage = new HomePage().userMenu.openAccountOverview();
+        accountOverviewPage.isExpectedPage();
+        var orderHistoryPage = accountOverviewPage.openOrderHistory();
        for (Product product : storage.products)


Be consistent for structures like:

for (Product product : storage.products){...}

Decide if we use Product product : or var product : and adjust it at all occurrences.
BTW: I would use Product product : since this is a little bit more readable if you are working on class fields.

occupant23

@oomelianchuk Please look into the comments

occupant23 · 2021-05-07T11:53:09Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

    {
        clearBrowserCookies();
        open(Neodymium.configuration().url() + url);
-        return new ProductdetailPage();
+        return new ProductDetailPage();


please call isExpectedPage() to validate the result

occupant23 · 2021-05-07T11:53:19Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

    {
        open(Neodymium.configuration().url() + url);
-        return new ProductdetailPage();
+        return new ProductDetailPage();


please call isExpectedPage() to validate the result

occupant23 · 2021-05-19T10:18:53Z

src/test/java/posters/cucumber/support/CartSupport.java

+        var cartPage = new CartPage();
+        cartPage.updateProductCountByName(name, style, size, amount);
+
+        var updateProduct = storage.getProductFromArrayList(name, size, style);


@oomelianchuk I think we can use storage.updateCountOfProduct() to shorten the code at this place.

You are right, done

occupant23 · 2021-05-19T10:23:12Z

src/test/java/posters/cucumber/support/GlobalStorage.java

@@ -38,8 +35,15 @@ public Product addProduct(Product product)
        // increase amount of product if already there or add the whole product
        if (products.contains(product))
        {
-            Product updatedProduct = products.get(products.indexOf(product));
-            updatedProduct.setAmount(updatedProduct.getAmount() + 1);
+            var updatedProduct = products.get(products.indexOf(product));


@oomelianchuk Please reuse updateCountOfProduct if possible. We should not implement the same functions again.

occupant23 · 2021-05-19T13:05:26Z

src/test/java/posters/cucumber/support/ProductSupport.java


 public class ProductSupport
 {
    @When("^product \"([^\"]*)\" is opened$")
-    public ProductdetailPage clickProductByName(String productName)
+    public ProductDetailPage clickProductByName(String productName)


@oomelianchuk is a return type needed at this place?
In general the Steps are not able to communicate with each other on this level, so please remove the return if it'n unused.

I couldn't find a reason, why we would need this. Removed

src/test/java/posters/cucumber/support/ProductSupport.java

src/test/java/posters/pageobjects/components/AddressForm.java

occupant23 · 2021-05-19T13:15:29Z

src/test/java/posters/pageobjects/pages/checkout/NewShippingAddressPage.java

@@ -60,42 +59,42 @@ public void validateStructure()
        $("#formAddDelAddr").shouldBe(visible);


@oomelianchuk Can we reuse the AddressForm at this place? und implement this functionality at one place?

occupant23 · 2021-05-19T13:18:37Z

src/test/java/posters/pageobjects/pages/checkout/PaymentPage.java

    {
        // Credit Card Number
        // Fills the card number field with the parameter
-        creditCardNumber.val(number);
+        creditCardNumber.val(card.getCardNumber());


@oomelianchuk Could we create a credit card form and reuse it at the possible places if the underlying form is the same?

occupant23 · 2021-05-19T13:22:50Z

src/test/java/posters/pageobjects/pages/checkout/PlaceOrderPage.java

+        String firstName = shippingAddress.getFirstName();
+        String lastName = shippingAddress.getLastName();
+        String fullName = firstName + " " + lastName;
+        shippingAddressForm.find(".name").shouldHave(exactText(fullName));


@oomelianchuk It would be nice to rename the shippingAddressForm to shippingAddressContainer or shippingAddressTile to avoid misunderstandings.

occupant23 · 2021-05-20T08:43:02Z

src/test/java/posters/cucumber/support/GlobalStorage.java

-            Product updatedProduct = products.get(products.indexOf(product));
-            updatedProduct.setAmount(updatedProduct.getAmount() + 1);
-            return updatedProduct;
+            updateCountOfProduct(product.getName(), product.getSize(), product.getStyle(), product.getAmount() + 1);


@oomelianchuk: if updateCountOfProduct would return the updated product we could save the second line of code

occupant23 · 2021-05-20T13:26:08Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

-        HomePage homePage = new HomePage();
-        homePage.isExpectedPage();
-        return homePage;
+        return new HomePage().isExpectedPage();
    };


@oomelianchuk Please remove the ; after functions. They are not needed.

occupant23 · 2021-05-20T13:27:08Z

src/test/java/posters/cucumber/support/OpenPageFlows.java

    }

-    @Step("open product page without cleared cookes")
-    public static ProductdetailPage openProductdetailsPage(String url)
+    // TODO check if needed - else delete


@oomelianchuk Please take care of the TODO

The method is called in OrderSupport.openProductPageAndAddItoTheCart, so I just removed TODO

occupant23 · 2021-05-20T13:33:14Z

src/test/java/posters/cucumber/support/RegisterSupport.java

-        registerUser(storage.user);
+        // registerUser(storage.user);
+        var registerPage = OpenPageFlows.registerPage();
+        registerPage.isExpectedPage();


@oomelianchuk the isExpectedPage method should already have been called by the registerPage method.
If this is the case please remove the second call.

The method is not called anywhere, so I removed it completely

occupant23 · 2021-05-20T13:33:27Z

src/test/java/posters/cucumber/support/RegisterSupport.java

+        var registerPage = OpenPageFlows.registerPage();
+        registerPage.isExpectedPage();
+        var loginPage = registerPage.sendRegisterForm(storage.user);
+        loginPage.isExpectedPage();


@oomelianchuk the isExpectedPage method should already have been called by the sendRegisterForm method.
If this is the case please remove the second call.

The method is not called anywhere, so I removed it completely

occupant23 · 2021-05-20T13:35:33Z

src/test/java/posters/cucumber/support/RegisterSupport.java

        storage.user = new User(firstName, lastName, eMail, password);
    };

    @Given("^login page is opened after registration$")
    public void registerUserSetup()
    {
        // use the user coming from dependency injection
-        registerUser(storage.user);
+        // registerUser(storage.user);


@oomelianchuk BTW: what happened to this registerUser(storage.user); function. Please utilize it or remove it and the comment if it isn't needed anymore.

The method is not called anywhere, so I removed it completely

src/test/java/posters/pageobjects/components/AddressForm.java

oomelianchuk · 2021-05-26T15:54:43Z

The method isPage added to AbstractPageObject and used in RegisterSupport.deleteUser

occupant23 · 2021-06-03T13:19:25Z

src/test/java/posters/cucumber/support/GlobalStorage.java

-            Product updatedProduct = products.get(products.indexOf(product));
-            updatedProduct.setAmount(updatedProduct.getAmount() + 1);
-            return updatedProduct;
+


remove unneeded new line

occupant23

@oomelianchuk go ahead please.

occupant23 · 2021-09-02T13:11:48Z

src/test/java/posters/cucumber/support/GlobalStorage.java

+    public void removeProduct(String name, String style, String size)
+    {
+        var updateProducht = getProductFromArrayList(name, size, style);
+        products.remove(products.indexOf(updateProducht));


is products.indexOf needed?

products.remove(updateProducht); should work as well

occupant23 · 2021-09-06T12:04:34Z

src/test/java/posters/cucumber/support/GlobalStorage.java

+
+    public void removeProduct(String name, String style, String size)
+    {
+        var updateProducht = getProductFromArrayList(name, size, style);


rename updateProducht to product.
Maybe you can also delete the local variable and call it directly within the next line.

occupant23 · 2021-09-06T12:05:21Z

src/test/java/posters/cucumber/support/GlobalStorage.java

+
+    public Product updateCountOfProduct(String name, String size, String style, int amount)
+    {
+        var updateProducht = getProductFromArrayList(name, size, style);


rename updateProducht to product.

occupant23 · 2021-09-28T09:55:44Z

src/test/java/posters/pageobjects/pages/AbstractPageObject.java

+    }
+
+    @Step("ensure the correct page was loaded")
+    public boolean isPage()


Not sure if this kind of isPage is a good variant. maybe this can't be generalized.
This will catch the error, but error artifacts (images/source code) will be created nevertheless.

I guess this requires a specific implementation using something like has(condition) to prevent side effects.

This is a tricky problem. I found 4 solutions for this but none of them is flawless.

Make page objects implement both isPage and isExpectedPage methods. - IMO, this is code duplication, as isExpectedPage is just a rephrasing of isPage but with assertion.

Turn off screenshots via com.codeborne.selenide.Configuration ( Configuration.screenshots=fasle; Configuration.savePageSource=fasle; ). The problem is that the com.codeborne.selenide.Configuration is not thread-safe, so, in case we use this approach, screenshots on failure will be disabled for all running tests for the time isPage method is executed.

Make page objects implement only isPage method and create the following standard implementation of isExpectedPage method:

@SuppressWarnings("unchecked") public <T extends AbstractPageObject> T isExpectedPage() { SelenideAddons.wrapAssertionError(() -> Assert.assertTrue("opened page is not the expected one", isPage())); return (T) this; }

This approach has multiple problems:

SuppressWarnings to make it possible for isExpectedPage method to return the page object

The exact information about failed assertion will be lost, which will make debugging more complicated

During object creation add elements to check in the isExpectedPage/isPage method in the pageCharacteristics map, defined in AbstractPageObject. AbstractPageObject can then contain the following methods:

@SuppressWarnings("unchecked") public <T extends AbstractPageObject> T isExpectedPage() { pageCharacteristics.forEach((element, condition) -> element.should(condition)); return (T) this; } @Step("ensure the correct page was loaded") public boolean isPage() { LinkedList<Boolean> results = new LinkedList<>(); pageCharacteristics.forEach((element, condition) -> results.add(element.has(condition))); return results.contains(false); }

This approach also has problems with casts but solves the problem with lost information about the failed assertion.
Instead, it introduces a relatively complicated and unclear structure to the project.

What approach do you prefer? Do you have any other ideas?

isPage method is removed

Uwe Friedel added 3 commits April 9, 2021 08:54

isExpectedPage() changed

bc0b98d

javadoc removed

eb3934e

type changed to var

e85219c

ufjena requested a review from occupant23 April 12, 2021 19:54

step annotations changed

82964a0

occupant23 suggested changes Apr 28, 2021

View reviewed changes

ufjena and others added 11 commits April 28, 2021 15:03

pageobjects changed

410e80c

dataobjects changed

11190ad

project reworked

f02720e

isExpectedPage() - double use removed

54cd243

order changed for easier debugging

db6ae4a

project reworked

ca7a5f9

project reworked

ebe2311

step annotations removed - test case added

832d1ed

RegisterPage changed

a6363ec

remove address form validation from AbstractBrowsingPage

f6701fe

improve usage of AddressForm

844c366

oomelianchuk self-assigned this May 19, 2021

occupant23 suggested changes May 19, 2021

View reviewed changes

oomelianchuk added 7 commits May 19, 2021 16:26

call isExpectedPage before returning page object

3588390

remove return types from support flows

04fcb79

remove duplicated functionality and use updateCountOfProduct instead

6810e52

use forms to avoid code duplication

28c9a88

rename ~AddressForm to ~AddressContainer

9be2a4e

improve toString method of Product

f35bbeb

improve naming of openProductdetailsPage

79f2e79

oomelianchuk requested a review from occupant23 May 19, 2021 15:32

occupant23 suggested changes May 20, 2021

View reviewed changes

oomelianchuk added 2 commits May 20, 2021 15:57

return Product in updateCountOfProduct avoid product fetch

4d84e10

remove ; from open page methods

6eb407c

oomelianchuk added 4 commits May 20, 2021 16:03

remove TODO, method call in OrderSupport.openProductPageAndAddItoTheCart

04ee4c9

remove unneeded isExpectedPage call

362ce7d

remove unused method

fa23796

improve isComponentAvailable method of AddressForm

446951c

oomelianchuk requested a review from occupant23 May 20, 2021 15:02

oomelianchuk added 2 commits May 26, 2021 17:40

add isPage method to check if the expected page is loaded without errors

4050880

check if account page is open while cleanup and don't open it again

d02ffb4

occupant23 reviewed Jun 3, 2021

View reviewed changes

remove unneeded new line

4c1865d

oomelianchuk requested a review from occupant23 June 4, 2021 11:05

occupant23 suggested changes Sep 28, 2021

View reviewed changes

improve removeProduct method

3dde365

oomelianchuk requested a review from occupant23 October 1, 2021 10:31

oomelianchuk added 3 commits October 1, 2021 14:23

fix toString method for Product

bbaf053

open mini cart before going to cart page

4a07108

remove isPage method

e6c496d

		@@ -60,42 +59,42 @@ public void validateStructure()
		$("#formAddDelAddr").shouldBe(visible);

Change type to var #6

Are you sure you want to change the base?

Change type to var #6

Conversation

ufjena commented Apr 12, 2021

occupant23 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

occupant23 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

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

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

oomelianchuk May 20, 2021 • 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

oomelianchuk commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

occupant23 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomelianchuk May 20, 2021 •

edited

Loading