Skip to content

Conversation

@EvgStep
Copy link

@EvgStep EvgStep commented Jan 24, 2024

No description provided.

4935_Test-refactoring-element-Sidenav
@EvgStep EvgStep changed the base branch from master to angular_rework_development January 24, 2024 09:09
@pnatashap pnatashap requested a review from spbqaru January 24, 2024 14:56
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.

не поверю, что с описанием самих классов все хорошо и ничего менять не надо

public static final String MODE = "mode";
public static final String SIDE = "side";

@BeforeMethod(alwaysRun = true)
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
@BeforeMethod(alwaysRun = true)
@BeforeClass(alwaysRun = true)

Copy link
Author

Choose a reason for hiding this comment

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

заменил

public static Button toggleFixedSideNav;

@UI("#fixed-position .mat-sidenav-content mat-form-field input[formcontrolname='top']")
public static UIElement topGap;
Copy link
Contributor

Choose a reason for hiding this comment

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

не должно быть UIElement на странице

Copy link
Author

Choose a reason for hiding this comment

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

убрал все UIElment


backDropToggle.click();
sidenavBackdropDrawer.is().displayed();
sidenavBackdropDrawer.has().cssClass("mat-drawer-end");
Copy link
Contributor

Choose a reason for hiding this comment

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

не нужно проверять наличие css классов, этот функционал прекрасно работает
если класс что-то значит - должен быть в описании класса

Copy link
Author

Choose a reason for hiding this comment

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

исправил

@pnatashap
Copy link
Contributor

почему прогон автотестов не запустился, тоже не понятно

@EvgStep EvgStep requested a review from pnatashap January 26, 2024 08:39
@EvgStep EvgStep force-pushed the 4935_Test-refactoring-element-Sidenav branch from e3e77f3 to ac9d151 Compare January 26, 2024 10:40
}

@JDIAction("Get '{name}' side nav")
public UIElement getSideNav() {
Copy link
Contributor

Choose a reason for hiding this comment

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

не понятно: в классе SideNav у нас есть метод, который просит SideNav по какому-то индексу и получает UIElement.
явно нужно переделать

Copy link
Author

Choose a reason for hiding this comment

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

исправил


toggleFixedSideNav.click();
fixedPosition.base().timer().wait(() -> fixedPosition.visualValidation(".mat-sidenav-content"));
fixedPosition.getSideNav().has().attr(STYLE, "top: 100px; bottom: 100px; box-shadow: none; visibility: " +
Copy link
Contributor

Choose a reason for hiding this comment

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

не надо проверять style, это не стабильно и базовый функционал, который проверяется в html пакете

Copy link
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.

все еще на месте

"100px;");

toggleFixedSideNav.click();
fixedPosition.base().timer().wait(() -> fixedPosition.visualValidation(".mat-sidenav-content"));
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
Author

Choose a reason for hiding this comment

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

исправил

toggleExtraText.click();
autoSizeSideNav.getMatDrawer().has().text(containsString("Toggle extra text"));
autoSizeSideNav.getMatDrawerContent().has().attr(STYLE, "margin-left: 294px;");
autoSizeSideNav.getMatDrawerContent().has().attr(STYLE, "margin-left: 303px;");
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
Author

Choose a reason for hiding this comment

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

убрал

@EvgStep EvgStep requested a review from pnatashap January 31, 2024 13:41
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.

Full component review is required.
Only components from the documentation should be described

@JDIAction("Get '{name}' side nav by '{0}' position value")
public SideNav getSideNav(String position) {
UIElement element = null;
for (UIElement e : getSideNavItems()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is good to know how to implement it via streams

Copy link
Author

Choose a reason for hiding this comment

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

fixed


@JDIAction("Get '{name}' side nav content")
public UIElement getEvents() {
return getContent().find(".example-events");
Copy link
Contributor

Choose a reason for hiding this comment

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

it is obvious that this is a part of example site only.

Copy link
Author

Choose a reason for hiding this comment

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

removed

}

@JDIAction("Get '{name}' side nav content")
public WebList getResponsiveResults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not generic, there is no p description in component description

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

а что именно исправлено?
элемент p не входит в описание sidenav, это просто какое-то содержимое, которое может быть другим, поэтому тут его быть не может


@JDIAction("Get '{name}' side nav items")
private WebList getSideNavItems() {
return this.finds(".mat-sidenav");
Copy link
Contributor

Choose a reason for hiding this comment

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

How it is possible to have sidenav inside sidenav?

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@EvgStep EvgStep requested a review from pnatashap February 6, 2024 08:52
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.

почему так много изменений отмечено исправлеными и никаких изменений нет?


public String getMode() {
return mode;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

обычно еще делают метод получения Enum по строковому значению

}

@JDIAction("Get '{name}' side nav content")
public WebList getResponsiveResults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

а что именно исправлено?
элемент p не входит в описание sidenav, это просто какое-то содержимое, которое может быть другим, поэтому тут его быть не может

}

@JDIAction("Get '{name}' side nav content")
public UIElement getContent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

в интро к проекту есть часть про наименования - там написано, что мы минимизируем количество методов с get***, т.е. пишем их в случае крайней необходимости


@JDIAction("Get '{name}' side nav by '{0}' position value")
public SideNav getSideNav(String position) {
String notFoundMessage = String.format(ATTRIBUTE_NOT_FOUND_MESSAGE, POSITION, position);
Copy link
Contributor

Choose a reason for hiding this comment

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

а зачем собирать строку, которая может и не понадобится?

private static final String POSITION = "position";
private static final String ATTRIBUTE_NOT_FOUND_MESSAGE = "Element with attribute %s %s not found";

private Predicate<UIElement> attributeMatches(String attributeName, String attributeValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

import static com.epam.jdi.light.asserts.core.SoftAssert.jdiAssert;
import static java.lang.String.format;

public class SideNaveAssert extends UIAssert<SideNaveAssert, SideNav> implements ITextAssert<SideNaveAssert> {
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
public class SideNaveAssert extends UIAssert<SideNaveAssert, SideNav> implements ITextAssert<SideNaveAssert> {
public class SideNavAssert extends UIAssert<SideNaveAssert, SideNav> implements ITextAssert<SideNaveAssert> {

private static final String LOCATION_ERROR_MESSAGE = "SideNavSection with location %s isn't on the right side";

@JDIAction(value = "Assert that '{name}' has section with location '{0}' on the right side", isAssert = true)
public SideNavContainerAssert sideNavSectionOnTheRight(Point locationOfSection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

зачем нам проверять, что-то по координатам?


toggleFixedSideNav.click();
fixedPosition.base().timer().wait(() -> fixedPosition.visualValidation(".mat-sidenav-content"));
fixedPosition.getSideNav().has().attr(STYLE, "top: 100px; bottom: 100px; box-shadow: none; visibility: " +
Copy link
Contributor

Choose a reason for hiding this comment

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

все еще на месте

@pnatashap pnatashap requested a review from vklonin February 17, 2024 01:30
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