Skip to content

Conversation

@Choi-JJunho
Copy link
Contributor

@Choi-JJunho Choi-JJunho commented Nov 17, 2023

🚀 작업 내용

LMS의 ✅ 학습 테스트 - MVC Response 페이지를 기준으로 실패하는 테스트 환경을 구성했습니다.

💬 리뷰 중점사항

브랜치 생성 권한이 없어 PR에 의견 남겨봅니다!

target 브랜치를 main이 아닌 mvc-response로 하는건 어떨까요?

예상 브랜치 구조는 https://github.com/woowacourse/spring-learning-test 를 참고했습니다.

Copy link
Contributor

@boorownie boorownie left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다!
기존 학습 테스트에서 변경된 부분에 대해서도 코멘트를 남겨주시면 감사하겠습니다!
그리고 작업 시 README 파일도 함께 변경이 필요할 것 같아요 :)

import org.springframework.ui.Model;

@Controller
public class MemberController {
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

@Choi-JJunho Choi-JJunho Nov 27, 2023

Choose a reason for hiding this comment

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

README 및 LMS 상에서 제시되고있는 컨트롤러 명이 MemberController여서 변경했습니다!

LMS

image

Comment on lines 9 to 12
public Model world() {
// TODO: /hello 요청 시 resources/templates/hello.html 페이지가 응답할 수 있도록 설정한다.
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

리턴타입이 String에서 Model로 변경되었네요! 혹시 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Model을 활용하는 예제를 생각하다가 잘못 작성한 부분인 것 같습니다 😅
String으로 다시 변경하겠습니다!

Comment on lines 14 to 17
public Person json() {
// TODO: /json 요청 시 {"name": "brown", "age": 20} 응답할 수 있도록 설정한다.
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

요청 url도 변경이 발생했네요! 혹시 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 부분도 마찬가지로 README 및 LMS 상에서 제시되고있는 URL path에 맞춰서 변경했습니다!

LMS

image

이에 ResponseJsonTest 테스트에 작성된 /person 경로 또한 /json으로 변경했습니다.

Comment on lines 24 to 32
@Test
void responseTemplatesHelloPage() {
var response = RestAssured
.given().log().all()
.when().get("/hello")
.then().log().all().extract();

assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

새로운 학습 테스트 👍

@Choi-JJunho
Copy link
Contributor Author

Choi-JJunho commented Nov 27, 2023

학습테스트를 확인하는 과정에서 한가지 추가 작업한 내용이 있어 코멘트 남깁니다!

image

현재 학습테스트 중 3. Template Engine에서 제공하고자 하는 학습 목표는 Thymeleaf와 @RequestParam, Model을 활용하여 컨트롤러에서 뷰로 값을 전달하는 것을 목표로 하고 있는 것으로 이해했습니다.

하지만 현재 hello.html을 이용한 실습을 수행한다면 @RequestParam 혹은 Model 객체를 이용할 여지가 없는 상황입니다.

...
<!-- 기존 hello.html -->
<body>
  Hello World!
</body>
...

thymeleaf가 적용된 html view를 띄우는 것을 목표로 학습테스트를 수정했고 이에 기존의 /template 경로와 관련된 코드를 제거했습니다.

...
<!-- 변경된 hello.html, template.html 과 유사 -->
<body>
  <p th:text="'Hello, ' + ${name} + '!'" />
</body>
...

혹시 이러한 방식이 초기에 의도한 바와 많이 벗어나게 되는지 궁금합니다

@Choi-JJunho
Copy link
Contributor Author

브라운 안녕하세요~! 변경된 구조에 맞춰 반영해봤습니다!

LMS 자료에 맞춰 코드를 재구성했습니다!
한가지 특이사항이 있어 따로 구분하여 코멘트 남겨봅니다!

특이사항

정적파일 로드와 관련하여 hello.html이 아닌 static.html을 이용하도록 문서를 변경했습니다.
기존에 /static 경로와 /templates 경로 모두 hello.html을 사용하다보니 학습테스트 수행 과정에서 혼동이 올 수도 있다는 우려가 있었습니다.
이에 2 Static Page 학습테스트에 대한 html 파일을 hello.html이 아닌 static.html으로 변경하였고 이에 대한 README 파일도 수정했습니다.

만약 반영된다면 이에 대한 LMS 자료 수정이 필요할 것 같아 별도로 특이사항으로 구분하여 말씀드립니다

Copy link
Contributor

@boorownie boorownie left a comment

Choose a reason for hiding this comment

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

제안 주신 부분 좋습니다! 그렇게 반영하시죠 👍
누락된 부분이 보여서 코멘트 드립니다!

.when().get("/person")
.then().log().all().extract();
.given().log().all()
.when().get("/person")
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
.when().get("/person")
.when().get("/json")

.then().log().all().extract();

assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
assertThat(response.as(Person.class).getName()).isEqualTo("Brown");
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
assertThat(response.as(Person.class).getName()).isEqualTo("Brown");
assertThat(response.as(Person.class).getName()).isEqualTo("brown");

- 테스트 메서드: `cholog.ResponseStaticTest.responseStaticPage`
- 수행 방법
- `resources/templates/hello.html` 을 이용하여 학습 테스트를 성공시키세요.
- `resources/templates/static.html` 을 이용하여 학습 테스트를 성공시키세요.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

- 테스트 메서드: `cholog.ResponseStaticTest.responseStaticPage`
- 수행 방법
- `resources/templates/hello.html` 을 이용하여 학습 테스트를 성공시키세요.
- `resources/templates/static.html` 을 이용하여 학습 테스트를 성공시키세요.
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

@boorownie boorownie left a comment

Choose a reason for hiding this comment

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

피드백 반영 확인했습니다.
감사함니다!

@boorownie boorownie merged commit 664b58f into cho-log:main Dec 14, 2023
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