-
Notifications
You must be signed in to change notification settings - Fork 1
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
test: 신청폼 희망분야 페이지 e2e 테스트 작성 #166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보고 많이 배웁니다... 저도 test를 거의 쓰지 않는 입장이라 보면서 배우고 있습니다. 아래의 내용에 대한 글을 잠시 끄적입니다.
테스트를 작성하는데 어느정도까지 작성을 해야 할지에 대해서 조금 고민이 들었습니다. 예를 들어서 "개발자(디자이너/기획)를 누르면 왼쪽 네비게이션바에 질문이 생긴다" 와 같은 유저 시나리오가 있다면, 개발자, 디자이너, 기획자 전부 테스트를 해야 할지에 대해서 고민이 들었으며, "2순위로 동일한 분야를 선택할 수 없다" 를 테스트 한다면, 나올 수 있는 모든 케이스를 해보는게 맞을지 고민이 들었습니다.
제 생각에는 test도 또한 모든 선택 사항입니다. 만약 모든 케이스를 고려하여 짜게 된다면, 그 또한 좋은 test코드, 더욱 견고한 test코드가 될 수 있겠죠. 하지만 만약 요구사항이 변경이 된다면 어떻게 되는지 한번 생각하면 좋을 것 같습니다.
추가적으로 저는 개발자만 짜더라도 한번에 같은 로직으로서 점검하는 코드를 짠다면 MC/DC를 만족하는코드를 작성할 수 있을 것이라고 생각합니다.
클래스명, 혹은 useId() 의 id 등으로 가져오는 방식은 가독성도 떨어지고 유지보수적으로도 좋은 방법이 아닐 수 있겠다는 생각이 들었습니다. 다만, 구별되는 점을 찾기 어려운 경우(1순위, 2순위가 동일하게 분야 버튼이 생기는 경우. 아래 사진 참고) 어쩔 수 없이 cypress에서 찾아주는 선택자로 사용할 수 밖에 없어지는 것 같습니다. 해당 부분은 우선적으로 선언문을 통해서 get()한 부분이 어떤 부분인지를 알려주도록 하였습니다만 더 좋은 방법이 있을지 고민이 됩니다. ( testid 를 넣는 방식이 좋아 보이긴 합니다..!)
클래스, id의 경우에는 유지보수에 좋지 않는 방식이 않다는 것에 저도 동의합니다. 다만, 저희가 사용하는 연산자는 get() 뿐 아니라 훨신 더 많은 연산자를 사용할 수 있음을 알면 좋을 것 같습니다. 그 element가 유니크한 것이라면 대부분의 use case에 대해 찾을 수 있도록 하였고, 만약 아니라면 왜 그 use case가 못찾는지, 코드가 잘못 짜여 있는지 확인하는 것도 좋아보입니다.
하나 생각난 것이 있다면... 아마 APP또한 하나의 매직 넘버와 같은 위치라고 생각합니다. 이들에 대해서 고민하자면, constants안에 있는 배열 중 0번째 배열의 것을 테스트 해보는 것으로 더욱 견고한 테스트를 작성할 수 있을 것이라고 생각합니다.
"checkLocalStorage", | ||
(key: string, expectedValue: string) => { | ||
cy.window().then((win) => { | ||
const actualValue = win.localStorage.getItem(key); | ||
expect(actualValue).to.equal(expectedValue); | ||
}); | ||
} | ||
); | ||
|
||
Cypress.Commands.add("checkAlert", (expectedValue: string) => { | ||
cy.on("window:alert", (str) => { | ||
expect(str).to.equal(expectedValue); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오... 먼가 flux 패턴같은 느낌.... redux같은 느낌의 코드이군요 ㅋㅋㅋ
@@ -0,0 +1,121 @@ | |||
describe("chapter", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
먼가... 이 또한 어떤 것임을 알려주는 describe면 좋겠군요...(저는 사실 회의를 참여 하지 않아서 제 3자라고 생각하고 보십쇼)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 해당 부분은 수정이 필요한게 맞습니다.. 저만 올바르게 작성하지 않았네요
|
||
describe("초기 상태(처음 진입시, 로컬 데이터가 없는 상태)에서", () => { | ||
it("질문 제목 네비게이션 클릭시 “필수 질문을 작성해주세요.” 알람창이 뜬다. ", () => { | ||
cy.get("button").contains("기본 인적 사항을 입력해주세요.").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 없이 개발할 때는 몰랐지만, 여기서 "기본 인적 사항을 입력해주세요"와 같은 string들은 constants라는 폴더에 한번에 다룬다면 나중에 2군데, 3군데 수정 없이 작동할 것 같다라는 생각이 드네요(이래서 따로 빼는거였나...?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요 조금 반복되는 부분이 있는 것 같습니다..!
반복되더라도 하드하게 작성한 이유는 예전 아래의 아티클을 읽고 이게 맞다고 생각을 했던 것 같습니다..
방금 다시 읽어보니 조금은 다른 문제를 다루고 있어서 제가 잘못 생각한 부분이 있는 것 같습니다.
해당 부분 수정해보겠습니다!
it("다음 버튼 클릭시 “필수 질문을 작성해주세요.” alert창이 뜬다. ", () => { | ||
cy.get("button").contains("다음").should("exist").click(); | ||
|
||
cy.checkAlert("필수 질문을 작성해주세요."); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오... false에 대한 내용을 작성하시는 꼼꼼함까지... ㄷㄷㄷㄷ
beforeEach(() => { | ||
const secondChapterAPPButton = cy.get('[for=":r1:"]'); | ||
secondChapterAPPButton.click(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흑흑... 이게 무엇을 의미하는건가요...? label을 기점으로 button에 대한 설명이 부족한가요??
추가로 next 것을 사용할 수 있답니다.(물론 input이 무조건 label뒤에 있어야하지만, 일반적으로 그렇게 짜니깐요...?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하하... next의 존재를 모르고 있어서 이런 코드가 나왔던 것 같습니다.. 🥲
수정해보겠습니다!
조금 더 명확한 설명으로 변경 #160
추가적으로 코드를 수정해보았습니다..!
|
cy.on("window:alert", (str) => { | ||
expect(str).to.equal(expectedValue); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 alert 창의 내용을 콘솔창에 출력되도록만 했는데 alert 창의 내용까지 검증하는 코드 굉장히 좋은 것 같아요.
it("다음 버튼 클릭시 “필수 질문을 작성해주세요.” alert창이 뜬다. ", () => { | ||
cy.get("@nextButton").should("exist").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기로는 beforeEach에서 다음 버튼을 찾지 못하면 cypress에서 에러를 출력하기 때문에 should로 추가 검증은 안해도 될 것 같네요. 다른 코드들도 변경하면 좋겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 맞습니다!
처음에 제가 should를 작성한 이유는 제가 봤던 레퍼런스 코드 때문이었습니다.
제 기억상 렌더링이 안된 상태에서 클릭을 하려고 하면 문제가 발생할 수 있어 렌더링을 기다리고자 should를 넣어야 한다고 했던 것 같은데,
제 코드에서 안넣은 곳도 있는데도 성공하는 것을 보니 크게 문제되지는 않는 것 같습니다..!
둘 중에서 하나로 통일하는 것이 좋아 보이긴 합니다! 한번 수정해보도록 하겠습니다
"checkLocalStorage", | ||
(key: string, expectedValue: string) => { | ||
cy.window().then((win) => { | ||
const actualValue = win.localStorage.getItem(key); | ||
expect(actualValue).to.equal(expectedValue); | ||
}); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로컬 스토리지를 확인하는 코드 좋네요. 저도 merge후에 사용하겠습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 추상화네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다~! 의문점에 대해 몇 가지 생각을 남겨봅니다. 참고만 하시고 다른 생각도 남겨주세요 :)
테스트를 작성하는데 어느정도까지 작성을 해야 할지에 대해서 조금 고민이 들었습니다. 예를 들어서 "개발자(디자이너/기획)를 누르면 왼쪽 네비게이션바에 질문이 생긴다" 와 같은 유저 시나리오가 있다면, 개발자, 디자이너, 기획자 전부 테스트를 해야 할지에 대해서 고민이 들었으며, "2순위로 동일한 분야를 선택할 수 없다" 를 테스트 한다면, 나올 수 있는 모든 케이스를 해보는게 맞을지 고민이 들었습니다.
저도 이 부분이 고민되었는데요, 너무 많은 케이스를 고민하기에는 비용이 많이 들고, 적은 케이스만 커버하기에는 테스트 커버리지가 낮아진다고 생각해요. 그래서 적절하게 비용과 커버리지를 고민해서 기본 사항은 한 두가지와 특이 케이스만 고려한다면 적절하게 비용과 커버리지를 챙길 수 있지 않을까 라는 생각이네요~ 무엇보다 정답은 없는 것 같습니다
클래스명, 혹은 useId() 의 id 등으로 가져오는 방식은 가독성도 떨어지고 유지보수적으로도 좋은 방법이 아닐 수 있겠다는 생각이 들었습니다. 다만, 구별되는 점을 찾기 어려운 경우(1순위, 2순위가 동일하게 분야 버튼이 생기는 경우. 아래 사진 참고) 어쩔 수 없이 cypress에서 찾아주는 선택자로 사용할 수 밖에 없어지는 것 같습니다. 해당 부분은 우선적으로 선언문을 통해서 get()한 부분이 어떤 부분인지를 알려주도록 하였습니다만 더 좋은 방법이 있을지 고민이 됩니다. ( testid 를 넣는 방식이 좋아 보이긴 합니다..!)
저는 e2e에서는 접근할 때, "사용자의 측면에서 테스트" 하는 것이라고 생각해요. 따라서 선택자의 코드도 실제 인간이 시각적으로 접근하듯이 짜면 좋겠다는 생각이 들었습니다. 예를 들어, 1순위의 label들이면,
"1순위" 라는 값이 있는 요소의 형제 요소들 중에 존재하는 라벨 중, APP을 text로 가지고 있는 요소
// 의사 코드
cy.get("h3").contains("1순위").sibling().contains("APP");
처럼 말이죠! 건규님의 생각도 궁금하네요~!
"checkLocalStorage", | ||
(key: string, expectedValue: string) => { | ||
cy.window().then((win) => { | ||
const actualValue = win.localStorage.getItem(key); | ||
expect(actualValue).to.equal(expectedValue); | ||
}); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 추상화네요 👍
|
주요 변경사항
신청폼 희망 분야 페이지(첫 페이지) e2e테스트 작성하였습니다.
기존에 작성해둔 유저 시나리오를 바탕으로 테스트케이스를 작성하였습니다.
테스트케이스의 경우 아래의 아티클을 많이 참고하여 중복되는 부분을 describe로 빼는 방식으로 작성하였습니다.
https://yozm.wishket.com/magazine/detail/2435/
테스트 케이스 보기
“필수 질문을 작성해주세요.”
알람창이 뜬다.“필수 질문을 작성해주세요.”
alert창이 뜬다.“필수 질문을 작성해주세요.”
alert창이 뜬다.“필수 질문을 작성해주세요.”
alert창이 뜬다.“필수 질문을 작성해주세요.”
alert창이 뜬다.테스트 결과 화면 보기
추가 의문점
테스트를 작성하는데 어느정도까지 작성을 해야 할지에 대해서 조금 고민이 들었습니다. 예를 들어서
"개발자(디자이너/기획)를 누르면 왼쪽 네비게이션바에 질문이 생긴다"
와 같은 유저 시나리오가 있다면, 개발자, 디자이너, 기획자 전부 테스트를 해야 할지에 대해서 고민이 들었으며,"2순위로 동일한 분야를 선택할 수 없다"
를 테스트 한다면, 나올 수 있는 모든 케이스를 해보는게 맞을지 고민이 들었습니다.클래스명, 혹은 useId() 의 id 등으로 가져오는 방식은 가독성도 떨어지고 유지보수적으로도 좋은 방법이 아닐 수 있겠다는 생각이 들었습니다. 다만, 구별되는 점을 찾기 어려운 경우(1순위, 2순위가 동일하게 분야 버튼이 생기는 경우. 아래 사진 참고) 어쩔 수 없이 cypress에서 찾아주는 선택자로 사용할 수 밖에 없어지는 것 같습니다. 해당 부분은 우선적으로 선언문을 통해서 get()한 부분이 어떤 부분인지를 알려주도록 하였습니다만 더 좋은 방법이 있을지 고민이 됩니다. (
testid
를 넣는 방식이 좋아 보이긴 합니다..!)해당 부분에서 1순위의 APP을 가져오고자 한다면
cy.get('label').contains('APP')
을 할 수 있지만, 2순위의 APP을 가져올 때도 동일한 로직을 사용해야 합니다. eq(1)로도 안잡히네요.. 혹시나 방법을 아신다면 커맨트 달아주세요!리뷰어에게...
관련 이슈
closes #160