서론

업무 중 동료 분의 코드를 보고 제가 작성한 코드 리뷰 내용의 일부입니다. 물론 본문과 같이 상세하게 작성하지 못했습니다. 그래도 좋은 테스트 코드에 있어서 휘발성 코드 리뷰로 지나치기 아쉬웠기 때문에 블로그에서 조금 상세하게 이야기하고자 합니다.

이 글에서 언급한 리팩토링을 하더라도 기능적, 성능적으로 나아지는 부분은 거의 없습니다. 다만, 테스트 피라미드에 부합하기 위해 수행되는 리팩토링은 유지보수 비용 측면에서 많은 이득을 챙길 수 있습니다.

테스트 피라미드
마틴파울러의 테스트 피라미드 https://martinfowler.com/bliki/TestPyramid.html

또한 일관되지 않은 코드가 프로덕션에 반영되었을 때 성장하는 팀에 합류한 신규입사자가 큰 혼란을 겪을 수 있습니다.

지금부터 NestJS로 작성된 API를 jest, supertest로 테스트 코드를 작성하며 많은 테스트를 써야하는 구조적은 테스트를 써도 되는 구조를 비교해보겠습니다.

 

테스트 역피라미드

아래 예제 코드는 주문의 상세 정보를 응답하는 API 입니다.

order.controller.ts

@Controller('order')
export class OrderController {
  constructor(private readonly orderService: OrderService) {}

  @Get('/:id')
  async getOrderById(@Param('id') id: number) {
    const orderDetail = await this.orderService.findOrderDetailById(id);

    if (!orderDetail) {
      return ResponseEntity.ERROR_WITH(
        '존재하지 않습니다.',
        HttpStatus.NOT_FOUND,
      );
    }

    return ResponseEntity.OK_WITH(orderDetail);
  }
}

order.service.ts

@Injectable()
export class OrderService {
  constructor(
    @InjectRepository(OrderEntity)
    private readonly orderRepository: Repository<OrderEntity>,
    @InjectRepository(DeliveryEntity)
    private readonly deliveryRepository: Repository<Delivery>,
  ) {}

  async findOrderDetailById(id: number): Promise<OrderDetail | undefined> {
    const delivery = await this.deliveryRepository.findOne({
      where: { order: id },
    });
    const order = await this.orderRepository.findOne({ where: { id } });

    if (delivery && order) {
      return OrderDetail.create(order, delivery);
    }
  }
}

orderService로부터 받아온 주문의 상세 정보가 없으면 NOT_FOUND를 응답하고, 존재하면 주문의 상세 정보를 반환하는 API입니다. 위 코드는 기능상 전혀 문제가 없는 코드입니다. 다만, controller, service 레이어 테스트에서 문제가 발생합니다.

order.controller.spec.ts

it('주문 정보와 배송 정보가 존재하면 주문 상세 정보를 반환한다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());
  const delivery = await deliveryRepository.save(new DeliveryEntity(order))
  
  // when
  const response = await request(app.getHttpServer())
    .get(`/order/${order.id}`)
    .send();

  // then
  expect(response.status).toBe(HttpStatus.OK);
  expect(response.body.data.id).toBe(order.id);
  expect(response.body.data.status).toBe(order.status);
  expect(response.body.data.payMethod).toBe(order.payMethod);
});

it('존재하지 않는 id 로 요청시 NotFound를 반환한다.', async () => {
  // given
  const wrongId = -1;

  // when
  const response = await request(app.getHttpServer())
    .get(`/order/${wrongId}`)
    .send();

  // then
  expect(response.status).toBe(HttpStatus.NOT_FOUND);
});

it('배송 정보가 없는 주문 id로 요청시 NotFound를 반환한다.', async () => {
 // .. do something
});

it.todo('주문 정보가 없는 id로 요청시 NotFound를 반환한다.', async () => {
 // .. do something
});

order.service.spec.ts

it('주문이 존재하면 주문의 상세 정보를 반환한다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());
  const delivery = await deliveryRepository.save(new DeliveryEntity(order));

  // when
  const orderDetail = await orderService.findOrderDetailById(order.id);

  // then
  expect(orderDetail.id).toBe(order.id);
  expect(orderDetail.delivery).toBe(delivery);
});

it('주문이 존재하지 않으면 undefined를 반환한다.', async () => {
  // given
  const wrongId = -1;

  // when
  const orderDetail = await orderService.findOrderDetailById(order.id);

  // then
  expect(orderDetail).toBeUndefined();
});

it('주문은 존재하지만 배송이 존재하지 않으면 undefined를 반환한다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());

  // when
  const orderDetail = await orderService.findOrderDetailById(order.id);

  // then
  expect(orderDetail).toBeUndefined();
});

위 테스트 코드를 보시다시피 controller e2e 테스트는 4개, service integration 테스트는 3개입니다. 하지만 테스트 피라미드에서 integration 테스트가 e2e 테스트보다 적어야 한다고 했는데 오히려 많습니다. 위와 같은 테스트 구조를 우스갯 소리로 Ice Cream Cone테스트라고 합니다.

Ice Cream Cone Test

controller는 변하기 쉬운 service 레이어에 의존하며 에러 처리를하고 있습니다. 따라서 service 로직이 수정되면 관련된 e2e, integration 테스트 전부 실패한다는 뜻입니다. 즉, 이 구조로 지속적인 요구사항 변화는 테스트 유지보수 비용을 증가시키고 Test ROI가 낮아지게 될 것입니다.

 

올바른 테스트 피라미드

기능의 변화 없이 리펙토링으로 테스트 코드 수를 줄여보겠습니다.

order.controller.ts

@Get('/:id')
  async getOrderById(@Param('id') id: number) {
    try {
      const orderDetail = await this.orderService.findOrderDetailById(id);

      return ResponseEntity.OK_WITH(orderDetail);
    } catch (e) {
      if (e instanceof EntityNotFoundError) {
        return ResponseEntity.ERROR_WITH(e.message, HttpStatus.NOT_FOUND);
      }

      this.logger.error(`OrderController getOrderById unknown error: id=${id}`, e);
      return ResponseEntity.ERROR_WITH(e.message, HttpStatus.SERVER_ERROR);
    }
  }

order.service.ts

async findOrderDetailById(id: number): Promise<OrderDetail> {
  // findOneOrFail 로 변경
  const delivery = await this.deliveryRepository.findOneOrFail({ 
    where: { order: id },
  });
  // findOneOrFail 로 변경
  const order = await this.orderRepository.findOneOrFail(id);

  return OrderDetail.create(order, delivery);
}

기존 에러 처리 로직을 try catch문으로 리팩토링 하였습니다. 테스트 코드는 아래와 같습니다.

order.controller.spec.ts

it('주문 정보와 배송 정보가 존재하면 주문 상세 정보를 반환한다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());
  const delivery = await deliveryRepository.save(new DeliveryEntity(order));

  // when
  const response = await request(app.getHttpServer())
    .get(`/order/${order.id}`)
    .send();

  // then
  expect(response.status).toBe(HttpStatus.OK);
  expect(response.body.data.id).toBe(order.id);
  expect(response.body.data.status).toBe(order.status);
  expect(response.body.data.payMethod).toBe(order.payMethod);
});

order.service.spec.ts

it('주문이 존재하면 주문의 상세 정보를 반환한다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());
  const delivery = await deliveryRepository.save(new DeliveryEntity(order));

  // when
  const orderDetail = await orderService.findOrderDetailById(order.id);

  // then
  expect(orderDetail.id).toBe(order.id);
  expect(orderDetail.delivery).toBe(delivery);
});

it('주문이 존재하지 않으면 EntityNotFoundError를 던진다.', async () => {
  // given
  const wrongId = -1;

  // when
  const t = async () => {
    await orderService.findOrderDetailById(wrongId);
  };

  // then
  await expect(t()).rejects.toThrow(EntityNotFoundError);
});

it('주문은 존재하지만 배송이 존재하지 않으면 EntityNotFoundError를 던진다.', async () => {
  // given
  const order = await orderRepository.save(new OrderEntity());

  // when
  const t = async () => {
    await orderService.findOrderDetailById(order.id);
  };

  // then
  await expect(t()).rejects.toThrow(EntityNotFoundError);
});

 

리팩토링 후 e2e 테스트 1개, integration 테스트 3개만 작성되었습니다. 최종적으로 테스트 피라미드 구조로 테스트 코드가 작성되었습니다. 테스트 코드에서 조금 달라진 점이 있다면 e2e 테스트에서 에러 케이스를 검증하지 않았습니다. 그 이유는 controller에 존재하는 로직은 service에서 error가 던져졌을 때 비즈니스 로직이 실행되지 않기 때문입니다. 그래서 e2e 테스트에서 모든 에러 케이스를 검증하지 않아도 됩니다.

만약 service에서 AError, BError를 던질 수 있는데 controller에서 AError만 처리했다면 어떻게 될까요? 그렇다면 개발자가 예상하지 못한 BError가 발생했기 때문에 명백한 SERVER_ERROR(500) 입니다. 그래서 order.controller.ts에서 catch문 하단부에서 개발자가 직접 캐치하지 못한 에러들을 500으로 응답합니다.

성능적으로 일부 개선된 점도 있습니다. 기존 order.service.ts에서 delivery가 존재하지 않아도 order를 조회하였습니다. 리팩토링 후 delivery가 조회되지 않았다면 즉시 에러가 던져지기 때문에 order를 조회하는 쿼리가 실행되지 않습니다. 쿼리 실행 횟수가 2회에서 1회로 줄었기 때문에 성능 개선이라 볼 수 있습니다.

'NestJS' 카테고리의 다른 글

[NestJS] JWT 로그인 구현 예제 (bcrypt, passport, JWT, cookie)  (4) 2021.12.29