Skip to content

Домашняя работа 7#11

Open
khitao wants to merge 4 commits into
developfrom
home_work_7
Open

Домашняя работа 7#11
khitao wants to merge 4 commits into
developfrom
home_work_7

Conversation

@khitao

@khitao khitao commented Nov 1, 2023

Copy link
Copy Markdown
Owner

Покрыл существующий код интеграционными тестами с weather API, добавил MVC-тесты на эндпоинт GET и POST, использовал TestContainers.

Покрыл существующий код интеграционными тестами с weather API, добавил MVC-тесты на эндпоинт GET и POST, использовал TestContainers.

@madoed madoed left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Доработать по комментариям( указал в первом месте, где встретилось, в нижеследующих не отмечал, но тоже надо поправить). Ответить на комментарии

@@ -1,9 +1,15 @@
weather.api.key=your_api_key
weather.api.key=c45a3b4355a94076bc8182632230610

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Секреты не коммитим в репозиторий

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Не уследил (


@AutoConfigureMockMvc(printOnlyOnFailure = false)
@WebMvcTest(WeatherApiController.class)
public class WeatherApiControllerIT {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Давай не сокращать IntegrationTest в "IT"


@SpringBootTest
@Testcontainers
public class WeatherTypeServiceImpl {

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" в имени тестового класса

Плюс тесты с Testcontainers падают у меня локально с Connection to localhost:5432 refused.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Так же есть предложение использовать Testcontainers в виде singleton -> https://java.testcontainers.org/test_framework_integration/manual_lifecycle_control/

Какие плюсы у этого подхода:

  • Запускаем контейнер с бд единожды
  • Прогон миграций 1 раз
  • Соответственно время прохождения тестов уменьшается

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Странно, у меня запускались 🤔 Сейчас посмотрю

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, либо программно https://www.baeldung.com/spring-dynamicpropertysource. Плюс для теста contextLoads отсуствует БД и ликвибейз, который у тебя включен по умолчанию не знает куда ему идти, т.к. по дефолтному пути (из основных application.properties) ничего не запущенно

Comment on lines +47 to +49
private static final String LOCATION = "Moscow";
private static final LocalDateTime DATE_TIME = LocalDateTime.now();
private static final double TEMPERATURE = 1.0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Статика должны быть выше не статики

https://www.oracle.com/technetwork/java/codeconventions-150003.pdf (4 страница)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Конвеншены могут быть разные, но обычно статика выше

Comment on lines +25 to +27
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

выбор использования BDD алиасов в Mockito был осознанным?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Да, я сначала не знал про BDD. А потом прочитал, что это намного лучше и читабельнее считается и решил тоже использовать.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

лучше что-то одно использовать, чтобы стиль был един в сервисе

Comment on lines +57 to +59
WeatherApi weatherApi = weatherApiClient.getCurrentWeather(LOCATION);

assertNotNull(weatherApi);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а зачем эта проверка?

@khitao khitao Nov 3, 2023

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Согласен, она здесь не нужна. В начале её запихнул, когда толко изучал тему.


assertNotNull(weatherApi);

when(weatherApiClient.getCurrentWeather(LOCATION)).thenReturn(weather);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Выше был уже созданный мок через BDD алиас

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Спешил( И не заметил, когда проверял просто до этого не использовал BDD, а тут решил поменять

Comment on lines +69 to +71
String request = mvcResult.getResponse().getContentAsString();

assertThat(request).isEqualToIgnoringWhitespace(objectMapper.writeValueAsString(weather));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Лучше через jsonPath https://www.baeldung.com/integration-testing-in-spring (пункт 4,2)

@Testcontainers
public class CityServiceImplTest {
@Container
public static PostgreSQLContainer<?> postgres = new PostgreSQLContainer<>("postgres:latest");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Вроде бы в задании говорилось о H2 контейнере

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Писали в беседе, что можно юзать и Postgres

Comment on lines +42 to +45
@BeforeEach
public void cleanUp() {
cityRepository.deleteAll();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ещё есть вариант использовать @Sql аннотацию поверх класса, чтобы делать тоже самое, но через sql скрипт

@madoed madoed left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Надо дочинить тесты, чтобы проходили

@@ -0,0 +1 @@
DELETE FROM city_weather; No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

а зачем скрипты удаления, кроме clean-up.sql?

Лучше чистить все данные, чтобы не было пересечений в последующих тестах. Ну и соответственно под каждый тест, создавать своё.

Иначе велика вероятность, что, кто-то сможет добавление теста, сломать что-то смежное

Copy link
Copy Markdown
Owner Author

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