Skip to content

1#1293

Open
Anastasiaaok wants to merge 5 commits intoyandex-praktikum:mainfrom
Anastasiaaok:develop1
Open

1#1293
Anastasiaaok wants to merge 5 commits intoyandex-praktikum:mainfrom
Anastasiaaok:develop1

Conversation

@Anastasiaaok
Copy link
Copy Markdown

No description provided.

Comment thread .idea/.gitignore Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Папку .idea не нужно было загружать в репозиторий. Эта папка должна быть добавлена в .gitignore.

Comment thread target/classes/praktikum/Bun.class Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Папку target не нужно было загружать в репозиторий. Эта папка должна быть добавлена в .gitignore.
⛔️Нужно исправить. Необходимо приложить jacoco отчет в пулл реквест

Comment thread src/test/java/praktikum/BurgerTest.java Outdated
this.expectedPrice = expectedPrice;
}

@Parameterized.Parameters
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️Можно улучшить. В параметризованных тестах для аннотации Parameterized.Parameters лучше использовать аргумент name: @Parameterized.Parameters(name = "Тестовые данные: {0} {1}"), где {0}, {1} - индексы параметров. Это повысит информативность проверки

Comment thread src/test/java/praktikum/BurgerTest.java Outdated

public BurgerTest(float bunPrice, float ingredientPrice1, float ingredientPrice2, float expectedPrice) {
this.bunPrice = bunPrice;
this.ingredientPrice1 = ingredientPrice1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. При нейминге не рекомендуется использовать числа (Field2), их еще называют magicNumbers. Очень тяжело поддерживать код с magicNumbers.


burger.addIngredient(ingredient);

assertEquals(1, burger.ingredients.size());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Для юнит-тестов придерживаемся подхода: один тест, значит одна проверка. Если очень хочется несколько проверок -- тогда используем softAssertions. Поправь, пожалуйста, во всем коде

Comment thread src/test/java/praktikum/BurgerTest.java Outdated
}

@Test
public void moveIngredientTest() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Если в тестах не нужна параметризация нужно вынести их в отдельный класс. Не нужно их запускать несколько раз

@Anastasiaaok
Copy link
Copy Markdown
Author

Добавляю JaCoCo отчет (скрин покрытия)
Jacoco

Comment thread pom.xml
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M7</version>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Необходимо приложить jacoco отчет в пулл реквест

Comment thread src/test/java/praktikum/BurgerTest.java Outdated
}

@Test
public void addIngredientTest_size() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Неверный нейминг. Не используем спецсимволы в названии методов

String receipt = burger.getReceipt();
assertTrue(receipt.contains("Булка"));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⛔️Нужно исправить. Нужно добавить параметризированный тест

}

@Test
public void removeIngredientShouldDecreaseSize() {
Copy link
Copy Markdown

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.

2 participants