-
Notifications
You must be signed in to change notification settings - Fork 47
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
#4926: Tests refactoring: Paginator element #5386
base: angular_rework_development
Are you sure you want to change the base?
#4926: Tests refactoring: Paginator element #5386
Conversation
@Kate-Semenova there are style errors, as you can see from the automatic checks |
facaf4f
to
1a7c77c
Compare
String expected = format("%d – %d of %d", from, to, total); | ||
@JDIAction(value = "Assert that range is '{0}' for '{name}'", isAssert = true) | ||
public PaginatorAssert rangeLabel(String label) { | ||
String expected = format(label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove format/ inline
} | ||
|
||
@Override | ||
@JDIAction(value = "Is '{name}' expanded", level = DEBUG, timeout = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we do not want to wait till it will be opened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied-pasted. ill remove waiters
|
||
public PaginatorSelector() { | ||
super(); | ||
cdkOverlayContainer.backdropSelectPanel = "div.mat-mdc-select-panel"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdkOverlayContainer.backdropSelectPanel = "div.mat-mdc-select-panel"; | |
cdkOverlayContainer.backdropSelectPanelLocaltor = "div.mat-mdc-select-panel"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose not to add any changes to the existing com.epam.jdi.light.angular.elements.composite.MaterialSelectorContainer, especially as it is in progress within PR #5096
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
неправильная иерархия и все
return this.core(); | ||
} | ||
|
||
@JDIAction("Get '{name}' selected value from selector") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result is toogle text. On previous method it is called toggle, now it is selected value, below we can see a method "selectedElement" and it has absolutely different search logic.
All the name must be aligned and clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
так и не исправлено
jdi-light-angular/src/main/java/com/epam/jdi/light/angular/elements/complex/Paginator.java
Outdated
Show resolved
Hide resolved
public PaginatorAssert is() { | ||
return new PaginatorAssert().set(this); | ||
@JDIAction("Get COLOR for selected value in the list of options for '{name}'") | ||
public String colorInList() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method name is not correct: according to the code we see that color is from selected element (typically they are differ)
} | ||
|
||
@JDIAction("Get if '{name}' has first and last page buttons shown") | ||
public boolean showFirstLastButtons() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show means an action, methods change nothing. Must be renamed
return next.isEnabled(); | ||
@JDIAction("Get COLOR theme for '{name}'") | ||
public AngularColors color() { | ||
final AngularColors color = AngularColors.fromName(core().attr("color")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all colors can exists in our table, it can be customer color, we should be still able to get it
public void next() { | ||
next.click(); | ||
@JDIAction("Get COLOR of selector`s boarder for '{name}'") | ||
public String colorOfBoarder() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public String colorOfBoarder() { | |
public String borderColor() { |
jdi-light-angular/src/main/java/com/epam/jdi/light/angular/elements/complex/Paginator.java
Show resolved
Hide resolved
private static final String PAGINATOR_PAGE_SIZE_SECTION_LOCATOR = ".mat-mdc-paginator-page-size"; | ||
private static final String ITEM_PER_PAGE_SELECTOR_LOCATOR = "mat-select"; | ||
private final PaginatorSelector itemPerPageSelector; | ||
private static final Pattern PATTERN = Pattern.compile("^(\\d+)( . (\\d+))? .+ (\\d+)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patter for what?
53a2f76
to
bc26b65
Compare
...ngular-tests/src/test/java/io/github/epam/angular/tests/elements/complex/PaginatorTests.java
Show resolved
Hide resolved
return this.core(); | ||
} | ||
|
||
@JDIAction("Get '{name}' selected value from selector") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
так и не исправлено
private static final int STEP = 100; | ||
private static final int PAGE_SIZE = 10; | ||
private static final int LENGTH = STEP * PAGE_SIZE - new Random().nextInt(STEP); | ||
private static final String RANGE_PATTERN = "%d - %d / %d"; | ||
|
||
@BeforeMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BeforeMethod | |
@BeforeClass |
public void labelPaginationTest() { | ||
paginator.has().label("Items per page:"); | ||
paginatorConfigurable.has().itemPerPageLabel("Items per page:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paginatorConfigurable.has().itemPerPageLabel("Items per page:"); | |
paginatorConfigurable.has().pageSizeLabel("Items per page:"); |
|
||
@JDIAction(value = "Assert that '{name}' has '{0}' color theme", isAssert = true) | ||
public PaginatorAssert colorTheme(final AngularColors value) { | ||
final AngularColors color = AngularColors.fromName(element().colorTheme()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
получение цвета должно быть в jdiAssert
и везде ниже тоже
} | ||
|
||
@JDIAction(value = "Assert that '{name}' has page index of {0}", isAssert = true) | ||
public PaginatorAssert totalNumberOfItems(int length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
кто такие Items в этом методе?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void previousEnabled() { | ||
jdiAssert(element().isPreviousEnabled(), Matchers.is(true), "ERROR MESSAGE IS REQUIRED"); | ||
public PaginatorAssert previousEnabled() { | ||
jdiAssert(element().previousButton().isEnabled() ? "enabled" : "disabled", Matchers.equalTo("enabled")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не надо сравнивать строки, сравниваем boolean и пишем нормальное сообщение об ошибке
и ниже тоже
} | ||
|
||
@JDIAction(value = "Assert that previous button enabled for '{name}'", isAssert = true) | ||
public void previousEnabled() { | ||
jdiAssert(element().isPreviousEnabled(), Matchers.is(true), "ERROR MESSAGE IS REQUIRED"); | ||
public PaginatorAssert previousEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public PaginatorAssert previousEnabled() { | |
public PaginatorAssert previousPageButtonEnabled() { |
} | ||
|
||
@JDIAction(value = "Assert that first page button displayed={0} for '{name}'", isAssert = true) | ||
public PaginatorAssert firstPageDisplayed(boolean value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
имя метода путает: в нем написано, что проверяем, оторбражается ли первая страница, а имеется ввиду отображается ли кнопка перехода к первой странице
344222c
to
d55cc1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
и повторить ООП
@@ -35,8 +35,12 @@ public static AngularColors fromStyle(String styleName) { | |||
} | |||
|
|||
public static AngularColors fromColor(String color) { | |||
final String finalColor = color.startsWith("rgb(") | |||
? color.replace("rgb(", "rgba(").replace(")", ", 1)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rgb мы заменяем на rgba только условно, а вот в конец мы в любом случае добавляем 1, т.е. если вход будет rgba, то он станет невалидным цветом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pnatashap В данном случае я не до конца понимаю, что нужно менять. Отталкиваюсь в первую очередь от того, что уже есть в фреймворке, а это енам, хардкорженные значения:
} | ||
|
||
@Override | ||
@JDIAction(value = "Check that '{name}' is enabled", timeout = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
в чем необходимость добавление 0вого timeout?
|
||
@Override | ||
@JDIAction("Expand '{name}'") | ||
public void expand() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
метод путает - иногда бывает, что в paginator есть возможность увидеть номера страниц посередине, например, а тут видимо открытие выпадающего списка с размером страницы, не очевидный метод, надо переиновывать
} | ||
|
||
@JDIAction("Get '{name}' selected UIElement from the list") | ||
public UIElement selectedOptionFromList() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а почему так непонятно Option?
мы вибираем размер страницы, вот так и должен называться метод
|
||
@Override | ||
@JDIAction("Get 'name' toggle") | ||
protected UIElement toggle() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
что за toggle? это просто этот элемент и ничего больше
|
||
public PaginatorSelector() { | ||
super(); | ||
cdkOverlayContainer.backdropSelectPanel = "div.mat-mdc-select-panel"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
неправильная иерархия и все
|
||
import java.util.NoSuchElementException; | ||
|
||
public class PaginatorSelector extends MaterialSelector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
проблема с построением класса: этот класс не расширяет MaterialSelector, а содержит (причем опционально его, выпадающий список с размером страницы вообще может отсутствовать)
удалить наследование и излишне приписанные методы и пересмотреть все элементы и методы
d55cc1d
to
1fc5b56
Compare
public static Input pageSizeOptionsInput; | ||
|
||
@UI("//paginator-configurable-example//div[contains(text(),'List length:')]") | ||
public static Text listLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a listLenght, it is a full message, so can be listLenghtMessage
@UI("//paginator-configurable-example//div[contains(text(),'List length:')]") | ||
public static Text listLength; | ||
@UI("//paginator-configurable-example//div[contains(text(),'Page size:')]") | ||
public static Text pageSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same as above
@UI("//paginator-configurable-example//div[contains(text(),'Page size:')]") | ||
public static Text pageSize; | ||
@UI("//paginator-configurable-example//div[contains(text(),'Page index:')]") | ||
public static Text pageIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same as above
private Pattern rangeLabelPattern = Pattern.compile("^(\\d+)( . (\\d+))? .+ (\\d+)"); | ||
|
||
@JDIAction("Set pattern for '{name}' range label") | ||
public void setRangeLabelPattern(String regex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
должно задаваться аннотацией и инициализироваться конструктором
range = new UIElement(); | ||
range.setLocator(rangeLocator); | ||
@JDIAction("Get item per page selector for '{name}'") | ||
public UIElement itemPerPageSelector() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
список должен быть вынесен в отдельный элемент, потому что в общей куче все эти методы по открыть, закрыть, получить список - выглядят странно
private static final String SELECT_PANEL_LOCATOR = "div.mat-mdc-select-panel"; | ||
private static final String ITEM_PER_PAGE_OPTIONS_LOCATOR = "mat-option"; | ||
private static final String ARIA_EXPANDED_ATTR = "aria-expanded"; | ||
private static final String COLOR_ATT = "color"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
достаточно стандартный атрибут и кажется у нас есть интерфейс HasColor
не вижу необходимости вносить в константы
|
||
public class PaginatorAssert extends UIAssert<PaginatorAssert, Paginator> { | ||
@JDIAction(value = "Assert that '{name}' has '{0}' label", isAssert = true) | ||
public void label(String label) { | ||
jdiAssert(element().label(), Matchers.is(label)); | ||
public PaginatorAssert pageSizeLabel(final String label) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
должна быть еще валидация с матчером, не только на равно
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
под матчером имелось ввиду явная передача матчера, так много где сделано
@JDIAction("Assert that '{name}' css '{0}' {1}") public A css(String css, Matcher<String> condition) { jdiAssert(element().css(css), condition); return (A) this; }
1fc5b56
to
772e85c
Compare
…gular-paginator-tests # Conflicts: # jdi-light-angular/src/main/java/com/epam/jdi/light/angular/asserts/PaginatorAssert.java # jdi-light-angular/src/main/java/com/epam/jdi/light/angular/elements/enums/AngularColors.java
@Kate-Semenova "Error: src/main/java/com/epam/jdi/light/angular/asserts/PaginatorAssert.java:[66,19] (coding) UnnecessaryParentheses: Unnecessary parentheses around expression." |
protected UIElement range; | ||
protected String rangeLocator = ".mat-paginator-range-label"; | ||
private Pattern rangeLabelPattern = Pattern.compile("^(\\d+)( . (\\d+))? .+ (\\d+)"); | ||
@JDropdown(root = "mat-form-field.mat-mdc-form-field-type-mat-select", value = "mat-select", list = "//ancestor::body//mat-option") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
неправильный локатор, mat-option если будут еще на странице, все сломается
public Paginator() { | ||
label = new UIElement(); | ||
label.setLocator(labelLocator); | ||
@JDIAction("Get label for '{name}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
комментарий не соответствует методу
container.select(String.valueOf(number)); | ||
@JDIAction("Select items per page option '{0}' for '{name}'") | ||
public void selectItemPerPageOption(int number) { | ||
itemPerPageSelect.select(" " + number + " "); | ||
} | ||
|
||
@JDIAction("Get selected option for '{name}'") | ||
public int selected() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
было бы логичным думать, что selected для листалки страниц будет возвращать номер текущей страницы, а не количество элементов на странице
public boolean isPreviousEnabled() { | ||
return previous.isEnabled(); | ||
@JDIAction("Click previous for '{name}'") | ||
public void previousPage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- неправильное именование, глагола нет, а мы куда-то переходим
- метод не добавляет чего-то умного, т.е. если кнопки нет или она неактивна, то все равно будет исключение, с тем же успехом мы просто сами можем кликнуть по кнопке
if (core().hasAttribute("color")) { | ||
return core().attr("color"); | ||
} | ||
itemPerPageSelect.expand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
этого элемента может не быть, будет странным получить исключение на отсутствие какого-то элемента, если ты просто просил цвет
} | ||
|
||
@JDIAction("Get if '{name}' has first and last page buttons shown") | ||
public boolean isFirstLastButtonsShown() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
в selenium принято Displayed
return lastPageButton().isDisplayed() && firstPageButton().isDisplayed(); | ||
} | ||
|
||
@JDIAction("Parse '{name}' range with pattern '{rangeLabelPattern}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
angeLabelPattern это паттерн, лучше строку показывать, чтобы она читалась в логе
public void itemsPerPageList(final List<Integer> options) { | ||
jdiAssert(element().options(), Matchers.is(options)); | ||
public PaginatorAssert itemsPerPageList(final List<String> options) { | ||
element().itemPerPageSelect.expand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
список останется открытым, так себе
} | ||
|
||
@JDIAction(value = "Assert that previous button enabled for '{name}'", isAssert = true) | ||
public void previousEnabled() { | ||
jdiAssert(element().isPreviousEnabled(), Matchers.is(true), "ERROR MESSAGE IS REQUIRED"); | ||
public PaginatorAssert previousPageButtonEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
помимо enabled элемент еще может и не существовать вообще
|
||
public class PaginatorAssert extends UIAssert<PaginatorAssert, Paginator> { | ||
@JDIAction(value = "Assert that '{name}' has '{0}' label", isAssert = true) | ||
public void label(String label) { | ||
jdiAssert(element().label(), Matchers.is(label)); | ||
public PaginatorAssert pageSizeLabel(final String label) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
под матчером имелось ввиду явная передача матчера, так много где сделано
@JDIAction("Assert that '{name}' css '{0}' {1}") public A css(String css, Matcher<String> condition) { jdiAssert(element().css(css), condition); return (A) this; }
Refactored tests for PaginatorTests
Added new ones to cover the following features:
firstPageLabel: string
lastPageLabel: string
color: ThemePalette
disabled: boolean
hidePageSize: boolean
length: number
pageIndex: number
showFirstLastButtons: boolean