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

Update selenium tests from organizations package #7312

Closed
wants to merge 1 commit into from

Conversation

SkorikSergey
Copy link
Contributor

What does this PR do?

We need rework selenium tests from organizations package:

  • delete unused user;
  • delete test methods with similar use cases;
  • create selenium test for checking organization creation;
  • stabilize methods from page objects.

What issues does this PR fix or reference?

#7121

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Nov 10, 2017
@@ -81,12 +81,12 @@ public void clickOnMenu(MenuItem menuItem) {
seleniumWebDriver.findElement(By.xpath(locator)).click();
}

public String getMenuCounterValue(MenuItem menuItem) {
public int getMenuCounterValue(MenuItem menuItem) {
String locator =
String.format(Locators.MENU_ITEM_XPATH, menuItem.title) + Locators.COUNTER_XPATH;
String counter = seleniumWebDriver.findElement(By.xpath(locator)).getText();
Copy link
Contributor

@dmytro-ndp dmytro-ndp Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can use regex to extract number from text:
seleniumWebDriver.findElement(By.xpath(locator)).getText().replaceAll("\\s*[(]([0-9]+)[)]\\s*", "$1");
It should simplify the code slightly.

@@ -81,12 +81,12 @@ public void clickOnMenu(MenuItem menuItem) {
seleniumWebDriver.findElement(By.xpath(locator)).click();
}

public String getMenuCounterValue(MenuItem menuItem) {
public int getMenuCounterValue(MenuItem menuItem) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add example of menu item and returning value into JavaDoc to explain the propose of method.

String locator =
String.format(Locators.MENU_ITEM_XPATH, menuItem.title) + Locators.COUNTER_XPATH;
String counter = seleniumWebDriver.findElement(By.xpath(locator)).getText();
counter = counter.trim().replace("(", "").replace(")", "");
return counter.equals("") ? "0" : counter;
return counter.equals("") ? 0 : Integer.parseInt(counter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to handle NumberFormatException error on Integer.parseInt(counter) to handle possible text more safer.

@@ -216,6 +217,8 @@ public boolean isSearchInputVisible() {

/** Clicks on Add Organization button. */
public void clickAddOrganizationButton() {
new WebDriverWait(seleniumWebDriver, ELEMENT_TIMEOUT_SEC)
.until(ExpectedConditions.visibilityOf(addOrganizationButton));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use static import to improve readability.

assertEquals(
navigationBar.getMenuCounterValue(ORGANIZATIONS), String.valueOf(organizationsCount));
// Test UI views of organizations list
assertEquals(navigationBar.getMenuCounterValue(ORGANIZATIONS), organizationsCount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

public void testParentOrganization() {
organizationListPage.clickOnOrganization(parentOrganization.getName());
@Test
public void parentOrganizationViewsTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the test name convention, test word goes to the start of method name to the contrary to test class name where test word goes to the end of name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "views" looks as redundant here.

*
* @author Ann Shumilova
*/
public class AdminOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(AdminOrganizationTest.class);
public class AdminOfParentOrganizationViewsTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we really need views word in test name? IMHO it can be omitted so as view is just another name of window with organization page, and it's obvious that selenium tests are testing GUI which consists of windows/views.

@@ -44,32 +41,28 @@
*
* @author Ann Shumilova
*/
public class AdminOfSubOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(AdminOfSubOrganizationTest.class);
public class AdminOfSubOrganizationViewsTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public class AdminOfSubOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(AdminOfSubOrganizationTest.class);
public class AdminOfSubOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add "parent-" prefix to the name to simplify test result review.

private static final Logger LOG = LoggerFactory.getLogger(AdminOfSubOrganizationTest.class);
public class AdminOfSubOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add "child-" prefix to the name to simplify test result review.

public void testParentOrganization() {
organizationListPage.clickOnOrganization(parentOrganization.getName());
@Test
public void parentOrganizationViewsTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, see my notices here and here.

public class BulkOrganizationDeletionTest {
private static final Logger LOG = LoggerFactory.getLogger(BulkOrganizationDeletionTest.class);
public class DeleteOrganizationByBulkTest {
private static final String ORG_NAME1 = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add "1" in the name to simplify test result analysis.

private static final Logger LOG = LoggerFactory.getLogger(BulkOrganizationDeletionTest.class);
public class DeleteOrganizationByBulkTest {
private static final String ORG_NAME1 = generate("organization", 5);
private static final String ORG_NAME2 = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add "2" in the name to simplify test result analysis.


// Tests delete:
// Tests Bulk feature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems there is "Bulk Delete" feature tested.

assertTrue(organizationListPage.isBulkDeleteVisible());

// Delete all organizations by Bulk feature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Bulk Delete" is the phrase and dividing it doesn't make sense.

organizationListPage.clickBulkDeleteButton();
confirmDialog.waitOpened();
assertEquals(confirmDialog.getTitle(), "Delete organizations");
assertEquals(
confirmDialog.getMessage(),
String.format("Would you like to delete these %s organizations?", 2));
String.format("Would you like to delete these %s organizations?", organizationsCount));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use static import to improve readability.

import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

/**
* Test validates organization creation and actions on it in the list of organizations.
* Test validates deleting organizations from the list of organizations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"deleting organizations" -> "deletion of organizations", or "organization deletion".

}

@AfterClass
public void tearDown() throws Exception {
testOrganizationServiceClient.deleteById(organization.getId());
for (OrganizationDto organization : organizations)
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

testOrganizationServiceClient.deleteById(organization1.getId());
testOrganizationServiceClient.deleteById(organization2.getId());
for (OrganizationDto organization : organizations)
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

@AfterClass
public void tearDown() throws Exception {
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

testOrganizationServiceClient.deleteById(childOrganization.getId());
testOrganizationServiceClient.deleteById(parentOrganization.getId());
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

testOrganizationServiceClient.deleteById(parentOrganization.getId());
testOrganizationServiceClient.deleteById(rootOrganization.getId());
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

assertTrue(organizationListPage.getValues(NAME).contains(ORGANIZATION_NAME));

// Create sub-organization
organizationListPage.clickOnOrganization(ORGANIZATION_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, divide separate steps by empty rows to improve readability.

}

@AfterClass
public void tearDown() throws Exception {
testOrganizationServiceClient.deleteByName(organizationName);
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.


// Test filter the organization by wrong name
organizationListPage.clearSearchInput();
organizationListPage.typeInSearchInput(ORGANIZATION_NAME + "test");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use obvious "wrong_name" string in search to avoid potential confusion.

organizationListPage.clearSearchInput();
assertEquals(organizationListPage.getOrganizationListItemCount(), organizationsCount + 1);
assertEquals(organizationListPage.getOrganizationListItemCount(), organizationsCount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

*
* @author Ann Shumilova
*/
public class UserOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(UserOrganizationTest.class);
public class MemberOrganizationViewsTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, see my comment here.

public class UserOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(UserOrganizationTest.class);
public class MemberOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "parent" prefix to name to simplify test result analysis.

private static final Logger LOG = LoggerFactory.getLogger(UserOrganizationTest.class);
public class MemberOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "child" prefix to name to simplify test result analysis.

testOrganizationServiceClient.deleteById(childOrganization.getId());
testOrganizationServiceClient.deleteById(parentOrganization.getId());
for (OrganizationDto organization : organizations)
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

public class OrganizationTest {
public class OrganizationMembersTest {
private static final String PRE_CREATED_ORG_NAME = generate("organization", 5);
private static final String NEW_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "new" prefix to the name to simplify test result analysis.

testOrganizationServiceClient.deleteByName(PRE_CREATED_ORG_NAME);
testOrganizationServiceClient.deleteByName(NEW_ORG_NAME);
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

organizationListPage.waitForOrganizationsToolbar();
organizationListPage.waitForOrganizationsList();
organizationListPage.clickOnOrganization(organization.getQualifiedName());
organizationPage.waitOrganizationName(PRE_CREATED_ORG_NAME);

// Add members to a members list ad 'Admin'
// Add members to a members list as 'Admin'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@@ -38,34 +38,32 @@
* @author Ann Shumilova
*/
public class RenameOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(RenameOrganizationTest.class);
private static final String PARENT_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "parent" prefix to the name to simplify test result analysis.

@@ -38,34 +38,32 @@
* @author Ann Shumilova
*/
public class RenameOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(RenameOrganizationTest.class);
private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "child" prefix to the name to simplify test result analysis.

private static final Logger LOG = LoggerFactory.getLogger(RenameOrganizationTest.class);
private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
private static final String NEW_PARENT_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "new-parent" prefix to the name to simplify test result analysis.

private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
private static final String NEW_PARENT_ORG_NAME = generate("organization", 5);
private static final String NEW_CHILD_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "new-child" prefix to the name to simplify test result analysis.

@@ -74,81 +72,69 @@ public void setUp() throws Exception {

@AfterClass
public void tearDown() throws Exception {
testOrganizationServiceClient.deleteById(parentOrganization.getId());
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

organizationPage.waitOrganizationTitle(parentOrganization.getName());

organizationPage.setOrganizationName("");
organizationPage.setOrganizationName(" ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only one possible invalid name? What about empty name, and name with special symbols like "#$@!&" ?

}

@Test(priority = 2)
public void testSubOrganizationRename() {
String organizationPath = NEW_PARENT_ORG_NAME + "/" + CHILD_ORG_NAME;
String path = NEW_PARENT_ORG_NAME + "/" + NEW_CHILD_ORG_NAME;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion, it's better to rename path variable to describe its propose explicitly compere to organizationPath variable.

public class AdminOfParentOrganizationTest {

private static final Logger LOG = LoggerFactory.getLogger(AdminOfParentOrganizationTest.class);
public class SystemAdminOrganizationViewsTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, see comment here.


private static final Logger LOG = LoggerFactory.getLogger(AdminOfParentOrganizationTest.class);
public class SystemAdminOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "parent" prefix to the name to simplify test result analysis.

private static final Logger LOG = LoggerFactory.getLogger(AdminOfParentOrganizationTest.class);
public class SystemAdminOrganizationViewsTest {
private static final String PARENT_ORG_NAME = generate("organization", 5);
private static final String CHILD_ORG_NAME = generate("organization", 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add "child" prefix to the name to simplify test result analysis.

testOrganizationServiceClient.deleteById(childOrganization.getId());
testOrganizationServiceClient.deleteById(parentOrganization.getId());
for (OrganizationDto organization : testOrganizationServiceClient.getAll())
testOrganizationServiceClient.deleteById(organization.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such code can affect organizations which are created in parallel threads of test execution.

public class UserEmptyOrganizationTest {
private static final Logger LOG = LoggerFactory.getLogger(UserEmptyOrganizationTest.class);

public class UserEmptyOrganizationViewsTest {
Copy link
Contributor

@dmytro-ndp dmytro-ndp Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, see comment here.

@SkorikSergey SkorikSergey removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 15, 2017
@SkorikSergey
Copy link
Contributor Author

Pull request for reworking selenium tests from organizations package is in #7389.

@SkorikSergey SkorikSergey deleted the updateOrganizationTests branch November 15, 2017 12:21
@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 15, 2017
@SkorikSergey SkorikSergey removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants