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

4935 test refactoring element sidenav #5375

Open
wants to merge 4 commits into
base: angular_rework_development
Choose a base branch
from

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

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

@@ -28,11 +57,12 @@ public class SideNavTests extends TestsInit {

@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 force-pushed the 4935_Test-refactoring-element-Sidenav branch from e3e77f3 to ac9d151 Compare January 26, 2024 10:40
@Override
public SideNaveAssert has() {
return is();
}

@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.

исправил

fixSideNav.click();
fixedPosition.getSideNav().has().attr(STYLE, "transform: none; visibility: visible; top: 100px; bottom: " +
"100px;");
"100px;");

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.

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

fixSideNav.click();
fixedPosition.getSideNav().has().attr(STYLE, "transform: none; visibility: visible; top: 100px; bottom: " +
"100px;");
"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.

исправил

public void verifyAutoSizeSideNav() {
autoSizeSideNav.show();
toggleAutoNav.click();
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.

убрал

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 content")
public WebList getResponsiveResults() {
return getContent().finds("p");
return this.finds("a");
}

@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

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.

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

fixSideNav.click();
fixedPosition.getSideNav().has().attr(STYLE, "transform: none; visibility: visible; top: 100px; bottom: " +
"100px;");
"100px;");

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.

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

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.

None yet

4 participants