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

[Test]: 테스트코드 리팩터링 #79

Closed
pengooseDev opened this issue Apr 26, 2024 · 2 comments
Closed

[Test]: 테스트코드 리팩터링 #79

pengooseDev opened this issue Apr 26, 2024 · 2 comments

Comments

@pengooseDev
Copy link
Contributor

Description

테스트코드 리팩터링

우선, 한글을 사용한 멋진 오픈소스 프로젝트에 응원의 글을 남깁니다. 👍

현재 코드에서 아래의 사항을 개선하면 더 멋진 코드가 될 것 같아 issue를 남깁니다. :)

  1. 테스트 로직 분리.
  2. each 메서드를 이용한 코드 반복 제거.
  3. 테스트 관심사별 describe 세분화.

하단에 리팩터링 예시코드를 남겨두었으니 편하게 피드백주시면 감사하겠습니다.

Possible Solution

Current Code

import { chosungIncludes } from './chosungIncludes';

describe('chosungIncludes', () => {
  it('should return true when "ㅍㄹㅌ" is entered for searching "프론트엔드"', () => {
    expect(chosungIncludes('프론트엔드', 'ㅍㄹㅌ')).toBe(true);
  });

  it('should return true when "ㅍㄹㅌ" is entered for searching "00프론트엔드"', () => {
    expect(chosungIncludes('00프론트엔드', 'ㅍㄹㅌ')).toBe(true);
  });

  it('should return false when "ㅍㅌ" is entered for searching "프론트엔드"', () => {
    expect(chosungIncludes('프론트엔드', 'ㅍㅌ')).toBe(false);
  });

  it('should return true when "ㅍㄹㅌㅇㄷㄱㅂㅈ" is entered for searching "프론트엔드 개발자"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷㄱㅂㅈ')).toBe(true);
  });

  it('should return true when "ㅍㄹㅌㅇㄷ ㄱㅂㅈ" is entered for searching "프론트엔드 개발자"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')).toBe(true);
  });

  // ...codes
});

Step1) it의 each 메서드 사용.

기대 효과:

  • 반복코드 제거.
  • 하드코딩 된 TestCase 분리.

Refactored

import { chosungIncludes } from './chosungIncludes';

describe('chosungIncludes', () => {
  const allTestCases = [
    { search: 'ㅍㄹㅌ', target: '프론트엔드', expected: true },
    { search: 'ㅍㄹㅌ', target: '00프론트엔드', expected: true },
    { search: 'ㅍㄹㅌㅇㄷㄱㅂㅈ', target: '프론트엔드 개발자', expected: true },
    { search: 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ', target: '프론트엔드 개발자', expected: true },
    { search: 'ㅍㅌ', target: '프론트엔드', expected: false },
    { search: '푸롴트', target: '프론트엔드', expected: false },
    { search: 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ', target: ' ', expected: false },
  ];

  it.each(allTestCases)(
    'should return $expected when $search is entered for searching $target',
    ({ search, target, expected }) => {
      expect(chosungIncludes(target, search)).toBe(expected);
    }
  );
});

Step2) 테스트 관심사에 따른 describe 세분화

현재 리팩터링 된 코드는, 하나의 describe 내에서 초성이 포함되어있다고 판단되는 경우초성이 포함되어있다고 판단되지 않는 경우를 전부 테스트한다는 아쉬움이 있습니다. 이는, 관심사에 따라 describe를 세분화하여 해결할 수 있습니다.

import { chosungIncludes } from './chosungIncludes';

describe('chosungIncludes', () => {
  describe('초성이 포함되어있다고 판단되는 경우', () => {
    const testCases = [
      { search: 'ㅍㄹㅌ', target: '프론트엔드' },
      { search: 'ㅍㄹㅌ', target: '00프론트엔드' },
      { search: 'ㅍㄹㅌㅇㄷㄱㅂㅈ', target: '프론트엔드 개발자' },
      { search: 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ', target: '프론트엔드 개발자' },
    ];

    it.each(testCases)('should return true when $search is entered for searching $target', ({ search, target }) => {
      const result = chosungIncludes(target, search);

      expect(result).toBe(true);
    });
  });

  describe('초성이 포함되어있다고 판단되지 않는 경우', () => {
    const testCases = [
      { search: 'ㅍㅌ', target: '프론트엔드' },
      { search: '푸롴트', target: '프론트엔드' },
    ];

    it.each(testCases)('should return false when $search is entered for searching $target', ({ search, target }) => {
      const result = chosungIncludes(target, search);

      expect(result).toBe(false);
    });
  });
});

Result

image

etc.

No response

@okinawaa
Copy link
Member

안녕하세요! 너무 좋은 의견 주셔서 감사합니다!

Step1) it의 each 메서드 사용.

it.each 메서드를 사용하는 방법도 고려해봤는데요, 코드는 이뻐지고 간결해지지만 코드를 읽는 개발자가 정말 이해하기 쉬울까? 라는 측면에서 의문점을 가졌었어요.

기존 코드는 위에서 부터, 아래로 눈을 내려가며 쉽게 읽히는 반면, it.each를 사용하면, fixture의 값들과 실제 그 fixture들이 사용되는 부분이 코드 물리적으로 거리가 있다보니 쉽게 읽히지 않는다고 생각했어요.

테스트 코드는 문서화의 역할도 크게 하고 있기 때문에 가독성이 좋게 작성되면 좋겠다는 소망을 해왔었습니다!

Step2) 테스트 관심사에 따른 describe 세분화

이 부분은 너무 좋은것 같아요. 관심사 별로 describe그룹을 나누어, 개발자의 인지 능력을 최소화하여 부담을 덜어줄 수 있을 것 같네요!

@pengooseDev
Copy link
Contributor Author

@okinawaa
each 메서드에 대한 멋진 고민과 철학을 배워갑니다. 감사합니다. :)

남겨주신 피드백대로 테스트 관심사에 따른 describe 세분화 작업 후 PR을 생성하겠습니다.


추가적인 궁금증

현재 다른 테스트코드들을 에 영어와 한글이 혼재되어있습니다.

describe('hasBatchim', () => {
  it('should return true for the character "값"', () => { // ⛳️
    expect(hasBatchim('값')).toBe(true);
  });

  // ...
});

describe('hasSingleBatchim', () => {
  it('홑받침을 받으면 true를 반환한다.', () => { // ⛳️
    expect(hasSingleBatchim('공')).toBe(true);
    // ...
  });
  
  // ...
});

궁금증:

  • describe(⛳️)가 영어인 경우한글인 경우가 혼재되어있는데, 해당 부분은 어떤 기준으로 나뉘어져 있는 것인지 궁금합니다.
    (작업은 우선 한국어로 진행하고, 남겨주신 피드백을 보고 수정하도록 하겠습니다.)

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

No branches or pull requests

2 participants