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

4914_cardTestsRefactoring #5231

Closed
wants to merge 28 commits into from

Conversation

MayaElf
Copy link
Contributor

@MayaElf MayaElf commented Nov 24, 2023

No description provided.

@MayaElf MayaElf self-assigned this Nov 24, 2023
@MayaElf MayaElf changed the title 4914_cardTestsRefRebased [WIP] 4914_cardTestsRefactoring Nov 24, 2023
@MayaElf MayaElf changed the title [WIP] 4914_cardTestsRefactoring 4914_cardTestsRefactoring Nov 27, 2023
@MayaElf MayaElf requested review from pnatashap and vklonin November 27, 2023 18:02

@JDIAction("Get '{name}' size")
public CardImageSize cardImageSize() {
UIElement e = core().find("//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

image is not required for Card

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ниже комментарий, что работаем с имиджем в Кард

}

@JDIAction("Get '{name}' buttons")
public WebList getButtons() {
return this.finds(".mat-button");
return this.finds("//button");
}

@JDIAction("Get '{name}' text")
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем нам в общем случае весь этот текст с заголовками и текстом?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

тогда раделю на два, для заголовка и тела. нужен ответ на вопрос, который чуть ниже, можно ли сделать в одном классе, или все-таки делать отдельные классы. спасибо

Copy link
Contributor

Choose a reason for hiding this comment

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

бесполезный метод, удалить

card.getButtonByNumber(1).is().text("LIKE");
card.getButtonByNumber(2).is().text("SHARE");
}

@Test
@Test(description = "Test verifies that after click button becomes focused")
public void cardButtonsClickTest() {
card.getButtonByText("LIKE").click();
card.getButtonByText("LIKE").has().cssClass("cdk-focused");
Copy link
Contributor

Choose a reason for hiding this comment

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

Это специальный класс для определения фокуса?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, для фокуса

}

@JDIAction("Get '{name}' image")
public UIElement getImage() {
return this.find(".mat-card-image");
return this.find("//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

Картинка может быть в заголовке и содержимом, этот метод будет что-то возвращать любое.
Правильнее явно сделать заголовок и содержимое отдельными классами и у каждого получать картинку

Copy link
Contributor Author

Choose a reason for hiding this comment

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

можно ли сделать в одном классе, например, getHeaderImage и getContentImage? Или все-таки делать отдельные классы. элемент небольшой ведь

Choose a reason for hiding this comment

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

@pnatashap почему правильнее разбивать картинки на те что в заголовке и в содержимом ? Мы же в тестах идем от карты card.has()... а не от внутренностей карты

Copy link

@igor-vlasiuk igor-vlasiuk Jan 10, 2024

Choose a reason for hiding this comment

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

@pnatashap

Правильнее явно сделать заголовок и содержимое отдельными классами и у каждого получать картинку

Как ты это видишь ? Отдельные классы HeaderCardImage и ContentCardImage на ровне с классами angular элементов com.epam.jdi.light.angular.elements ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Зачем отдельные классы для картинок? Чем оно отличается от обычного img?
  2. https://material.angular.io/components/card/overview#basic-card-sections вот тут описана вся структура, которая может быть у карточки, вот эту структуру и должны читать и отдавать. но когда пишем локаторы, понимаем, что простые элементы могут быть где угодно, поэтому локатор должен быть адекватным
  3. не все части, описанные в документации, обязательно должны быть, это надо иметь ввиду

}

@JDIAction("Get '{name}' buttons")
public WebList getButtons() {
return this.finds(".mat-button");
return this.finds("//button");
Copy link
Contributor

Choose a reason for hiding this comment

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

кнопки могут быть в заголовке, в тексте в действиях, неправильно работать с ними со всеми
https://material.angular.io/components/card/examples - можно открыть редактор и проверять в онлайне

<mat-card class="example-card"> <mat-card-header> <mat-card-title>Shiba Inu</mat-card-title> <mat-card-subtitle>Dog Breed</mat-card-subtitle> <button mat-button>LIKE</button> </mat-card-header> <mat-card-content> <p>This card indeterminates progress bar.</p> <p>{{longText}}</p> <button mat-button>LIKE</button> </mat-card-content> <mat-card-actions> <button mat-button>LIKE</button> <button mat-button>SHARE</button> </mat-card-actions> </mat-card>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

может быть, добавить в примеры Кард с кнопками в заголовке?

Copy link

@igor-vlasiuk igor-vlasiuk Jan 9, 2024

Choose a reason for hiding this comment

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

На сколько я вижу, кнопка LIKE во всех примерах находится в теге mat-card-actions, не в mat-card-header.
@pnatashap покажи пожалуйста скриншотом, где кнопка находится в mat-card-header ?

Copy link
Contributor

Choose a reason for hiding this comment

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

кнопка может находится где угодно и мы об этом не узнаем, наша задача возвращать только кнопки из actions и НЕ брать никакие другие

@MayaElf MayaElf changed the title 4914_cardTestsRefactoring [WIP]4914_cardTestsRefactoring Dec 4, 2023
@MayaElf MayaElf requested a review from pnatashap December 8, 2023 08:55
@MayaElf MayaElf changed the title [WIP]4914_cardTestsRefactoring 4914_cardTestsRefactoring Dec 8, 2023
@pnatashap pnatashap removed their request for review December 9, 2023 03:44
@MayaElf MayaElf force-pushed the 4914_refactoringCardTests branch from 4fbd5a4 to 5256725 Compare December 22, 2023 09:27
@igor-vlasiuk igor-vlasiuk self-assigned this Jan 9, 2024
@MayaElf MayaElf removed their assignment Jan 9, 2024
@igor-vlasiuk igor-vlasiuk requested a review from pnatashap January 9, 2024 12:55
}

@JDIAction("Get '{name}' buttons")
public WebList getButtons() {
return this.finds(".mat-button");
return this.finds("//button");
Copy link
Contributor

Choose a reason for hiding this comment

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

кнопка может находится где угодно и мы об этом не узнаем, наша задача возвращать только кнопки из actions и НЕ брать никакие другие

public UIElement getHeaderText() {
return this.find(".mat-card-header-text");
@JDIAction("Get '{name}' avatar")
public UIElement getAvatar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

у аватара есть класс

Choose a reason for hiding this comment

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

класс Avatar я нашел в другом модуле io.github.com.custom;
Из него не импортируется

Copy link
Contributor

Choose a reason for hiding this comment

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

подумала, что есть, как в остальных фреймворках
все равно, открываем описание класса на сайте ангуляра:
<img mat-card-avatar> - An image used as an avatar within the header
Это не UIElement, это Image!

Choose a reason for hiding this comment

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

Поменял на Image

Copy link
Contributor

@pnatashap pnatashap left a comment

Choose a reason for hiding this comment

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

конфликт слияния, ни одна автопроверка не проверена

Igor Vlasiuk added 3 commits January 15, 2024 11:16
Copy link
Contributor

@pnatashap pnatashap left a comment

Choose a reason for hiding this comment

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

Продумай структуру основного компонента, а потом пиши под него код.
Текущий ПР практически полностью переписывать надо


@JDIAction("Get '{name}' size")
public CardImageSize cardHeaderImageSize() {
UIElement image = core().find("//mat-card-header//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

а почему есть метод, который даст размер первой попавшейся картинки заголока? а просто картинку получить нельзя?

Choose a reason for hiding this comment

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

Не понимаю, что значит "просто" картинку ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Получить Image, чтобы проверить url, alt, размер в пикселях. мы не только размер должны проверять

Choose a reason for hiding this comment

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

Для проверки url, alt есть отдельный тест attributeCardTest(), там проверка этих атрибутов только для одной картинки. Хочешь ли ты сказать что url, alt и еще размер в пикселях нужно проверять для каждой картинки ?

В задаче, по которой этот ПР сделан, #4914
не написано про url, alt и размер в пикселях. Можешь ли ты отредактировать задачу так как считаешь нужным ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. При чем тут тесты? Описание элемента еще не готово, я тесты не смотрела даже
  2. Бывает картинка в заголовке - аватар, бывает картинка в карточке - это другая картинка. Причем обе могут быть, а могут не быть (в этом ревью уже много комментариев по этому поводу). Если мы тестируем как выглядит страничка, то мы должны уметь проверять все эти компоненты и не только по тем размерам (которые быть вообще не обязаны), но и все, что мы обычно с картинкой делаем - альтернативный текст проверить, url, размеры, все что угодно. НЕ НАДО делать на все это отдельные методы валидации, потому что они уже есть у класса Image и проверены в html пакете


@JDIAction("Get '{name}' size")
public CardImageSize cardImageSize() {
UIElement image = core().find("//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

зачем нам метод который получает размер первой попавшейся картинки из карты, но саму картинку получить нельзя?

Choose a reason for hiding this comment

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

Не понимаю, что значит "просто" картинку ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Получить Image, чтобы проверить url, alt, размер в пикселях. мы не только размер должны проверять

Choose a reason for hiding this comment

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


public CardImageSize defineSize(UIElement image) {
CardImageSize size = CardImageSize.UNKNOWN;
if (image.hasClass("mat-mdc-card-xl-image")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

есть метод, который позволяет найти класс по регулярному выражению, используй его.

Choose a reason for hiding this comment

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

Исправил


@JDIAction("Get '{name}' size")
public CardImageSize cardImageSize() {
UIElement image = core().find("//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

Получить Image, чтобы проверить url, alt, размер в пикселях. мы не только размер должны проверять


@JDIAction("Get '{name}' size")
public CardImageSize cardHeaderImageSize() {
UIElement image = core().find("//mat-card-header//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

Получить Image, чтобы проверить url, alt, размер в пикселях. мы не только размер должны проверять

public Image image() {
core().show();
if (core().find("//img").isExist()) {
return new Image().setCore(Image.class, core().find("//img"));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. повторяющийся локатор
  2. уже несколько раз писала, что в карточке картинок может быть много, согласно документации в структуре описана только одна, вот локатор до нее мы и должны использовать

Choose a reason for hiding this comment

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

find("//img[" + imageNumber + "]")

public UIElement getHeaderText() {
return this.find(".mat-card-header-text");
@JDIAction("Get '{name}' avatar")
public UIElement getAvatar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

подумала, что есть, как в остальных фреймворках
все равно, открываем описание класса на сайте ангуляра:
<img mat-card-avatar> - An image used as an avatar within the header
Это не UIElement, это Image!

@pnatashap pnatashap requested a review from spbqaru January 18, 2024 19:24

@JDIAction("Get '{name}' header image")
public Image cartHeaderImage(int imageNumber) {
core().show();
Copy link
Contributor

Choose a reason for hiding this comment

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

нельзя такое делать в методе получения

Choose a reason for hiding this comment

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

окей


@JDIAction("Get '{name}' image")
public Image image(int imageNumber) {
core().show();
Copy link
Contributor

Choose a reason for hiding this comment

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

нельзя менять состояние страницы без спроса

Choose a reason for hiding this comment

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

ok

}

@JDIAction("Get '{name}' image")
public Image image(int imageNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

не нужен такой метод, мы работаем только с документированными элементами структуры

Choose a reason for hiding this comment

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

ну просто раньше ты писала что нужен метод, возвращающий конкретную картинку.
Специального id у картинки нет. Как тогда нужно захватывать картинку ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. ну вот смотри, у тебя есть одна карточка, у нее картинка в заголовке и в теле, вторая карточка с двумя картинками в залоговке и одной в теле. ты хочешь проверять картинку из тела (допустим первая всегда какая-то специконка). доступ по номеру тебе ничем не поможет, тебе нужна первая картинка из тела, а не в карточке.
  2. я уже перед этим писала, что мы описываем ТОЛЬКО структуру, которая подразумевается фреймворком.

Choose a reason for hiding this comment

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

image
Картинка находится не в теле, а непосредственно в карточке

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. в чем вопрос?
  2. а локатор у тебя в коде на что указывает?

Copy link
Contributor

@spbqaru spbqaru Jan 25, 2024

Choose a reason for hiding this comment

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

  1. в чем вопрос?
  2. а локатор у тебя в коде на что указывает?

Наталья, ты имеешь ввиду что локатор //img слишком универсальный?
тогда нужно взять картинку из структуры карточки mat-card>img прямой наследник от mat-card

либо из описания структуры (img mat-card-image Card image. Stretches the image to the container width)|
image

img.mat-mdc-card-image
чтобы исключить вероятность взять картинку из header.

и метод надо переназвать в cardBodyImage по аналогии с cartHeaderImage?

Copy link
Contributor

Choose a reason for hiding this comment

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

зачем методы в карточке называть card**** ??? мы и так в ней, зачем еще раз писать?

Choose a reason for hiding this comment

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

убрали card***

}

@JDIAction("Get '{name}' image")
public Image image(int imageNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. в чем вопрос?
  2. а локатор у тебя в коде на что указывает?


@JDIAction("Get '{name}' header image")
public Image cartHeaderImage() {
UIElement potentialImage = core().find("//mat-card-header//img");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UIElement potentialImage = core().find("//mat-card-header//img");
UIElement potentialImage = core().find(".//mat-card-header//img");

Choose a reason for hiding this comment

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

изменил


@JDIAction("Get '{name}' image")
public Image cardBodyImage() {
UIElement potentialImage = core().find("//img[@mat-card-image]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UIElement potentialImage = core().find("//img[@mat-card-image]");
UIElement potentialImage = core().find(".//img[@mat-card-image]");

Choose a reason for hiding this comment

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

изменил

if (potentialImage.isExist()) {
return new Image().setCore(Image.class, potentialImage);
} else {
throw runtimeException("Element doesn't have image", this);
Copy link
Contributor

Choose a reason for hiding this comment

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

не надо тут кидать исключение, мы возвращаем элемент, далее у него можно проверить, жив ли он

Choose a reason for hiding this comment

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

Не понимаю, что значит "жив" ? в этом методе мы хотим получить картинку. Если её нет, то что мы дальше можем проверять ?

@@ -57,17 +64,76 @@ public String getCardText() {
}

@JDIAction("Get button with text '{text}'")
public UIElement getButtonByText(String text) {
return this.getButtons().get(text);
public Button getButtonByText(String text) {
Copy link
Contributor

Choose a reason for hiding this comment

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

и здесь тоже, убрать get из названия, как и соседних методов

Choose a reason for hiding this comment

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

убрал


@JDIAction("Assert that {name} has image with '{1}' height and '{2}' width")
public CardAssert imageSize(int height, int width) {
jdiAssert(element().cardBodyImage().height(), Matchers.equalTo(height), "HEIGHT IS DIFFERENT");
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Зачем capslock сообщение об ошибке?
  2. Сообщение об ошибке вообще не информативно, не понятно сколько по факту, сколько ждали
  3. Зачем вообще кастомное сообщение?

Choose a reason for hiding this comment

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

  1. Делал по аналогии с другими методами в этом классе.
  2. Разве jdiAssert не выводит ожидаемое и по факту значения ?
  3. Делал по аналогии с предыдущими методами в этом классе.

Choose a reason for hiding this comment

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

Убрал кастомные сообщения

@JDIAction(value = "Assert that '{name}' has text '{0}'", isAssert = true)
public CardAssert assertCardText(String value) {
public CardAssert cardText(String value) {
jdiAssert(element().getCardText().equals(value), Matchers.is(true), "ERROR MESSAGE IS REQUIRED");
Copy link
Contributor

Choose a reason for hiding this comment

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

ERROR MESSAGE IS REQUIRED был вставлен автозаменой, чтобы check style не ругался

Choose a reason for hiding this comment

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

окей

@JDIAction("Assert that '{name}' has align end actions")
public CardAssert alignEndActions() {
jdiAssert(element().actionsEndAlign(), Matchers.is(true),
"Card actions are in end align position");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Card actions are in end align position");
"Card actions are in start align position");

Copy link
Contributor

Choose a reason for hiding this comment

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

сообщение выводится в случае ошибки, а значит там false, значит все наоборот. второе тоже исправить надо

Choose a reason for hiding this comment

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

исправил

@JDIAction("Assert that {name} has image '{1}' size")
public CardAssert headerImageSize(CardImageSize size) {
final CardImageSize actualCardImageSize = element().cardHeaderImageSize();
jdiAssert(actualCardImageSize, Matchers.is(size));
Copy link
Contributor

Choose a reason for hiding this comment

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

зачем actualCardImageSize запрашивать отдельно?

}

@JDIAction("Get '{name}' buttons")
public WebList getButtons() {
return this.finds(".mat-button");
return this.finds("//button");
}

@JDIAction("Get '{name}' text")
Copy link
Contributor

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 align end actions")
public boolean actionsEndAlign() {
UIElement e = core().find("mat-card-actions");
Copy link
Contributor

Choose a reason for hiding this comment

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

есть метод cardActions для получения этого объекта, не надо дублировать код

Choose a reason for hiding this comment

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

использовал

}

@JDIAction("Get '{name}' size")
public CardImageSize cardHeaderImageSize() {
Copy link
Contributor

Choose a reason for hiding this comment

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

а почему есть метод только размера картинки в заголовке?

Choose a reason for hiding this comment

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

потому что картинка в заголовке

@igor-vlasiuk
Copy link

because of rework

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants