chore: migrate tests from karma to jest #68

jajugoguma · 2021-06-08T12:50:10Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

Description

remove karma-jasmine test environment
set jest test environment
tests migrate from jasmine to jest

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

chore: remove karma-jasmine test environment

jung-han · 2021-06-09T05:14:10Z

jsconfig.json

@@ -0,0 +1 @@
+{ "typeAcquisition": { "include": [ "jest" ] } }


이건 어느 용도로 사용하는걸까요?_?

이 내용과 아래 @types/jest는 vscode 상에서 개발 시 Jest 문법의 자동 완성을 위해 사용했던 코드와 패키지 입니다.
제가 꼼꼼하게 확인하지 않고 커밋을 했습니다...
해당 파일과 의존 삭제하겠습니다

jung-han · 2021-06-09T05:16:43Z

test/domutil.spec.js

+    document.body.innerHTML += '<div id="_test" data-test="good"></div>' +
            '<span id="_test2" data-user-id="123"></span>' +
-            '<div id="_test3" data-user-name="hong"></div>');
+            '<div id="_test3" data-user-name="hong"></div>';


~~+ 없애고 템플릿 리터럴 이용해도 되겠네요~~

jung-han · 2021-06-09T05:18:27Z

package.json

    "@babel/plugin-transform-member-expression-literals": "^7.8.3",
    "@babel/preset-env": "^7.8.3",
    "@toast-ui/release-notes": "^2.0.1",
+    "@types/jest": "^26.0.23",


이 의존이 쓰이나요?_?

타입스크립트로 작성된게 아니라면 필요없을 것 같아요!

해당 의존 삭제하도록 하겠습니다!

jung-han · 2021-06-09T05:18:35Z

package.json

-    "karma-webdriver-launcher": "git+https://github.com/nhn/karma-webdriver-launcher.git#v1.2.0",
-    "karma-webpack": "^4.0.2",
+    "jest": "^27.0.4",
+    "jest-extended": "^0.11.5",


이건 혹시 어디에 쓰이나요?

// ajax.spec.js 235 expect(completeMock).toHaveBeenCalled(); 236 expect(successMock).toHaveBeenCalledBefore(completeMock); 237 });

위 코드 블록 등에서 toHaveBeenCalledBefore matcher를 사용하는데 쓰입니다.

해당 구문이 포함된 테스트는 complete 함수가 실행되기 전에 success 함수가 실행되기 전에 실행되는 것을 검증(다른 테스트도 마찬가지로)하기 때문에 해당 matcher를 사용해야 한다고 판단했습니다.

Jasmine은 toHaveBeenCalledBefore matcher를 기본 지원하지만, Jest는 그렇지 않기 때문에 이를 사용하기 위해 사용했습니다.

js87zz · 2021-06-09T05:40:44Z

setup-globals.js

@@ -0,0 +1,2 @@
+// eslint-disable-next-line strict
+global.Element = Element;


jsDOM환경이면 이 설정없이 잘 동작하지 않나요?

jsDOM 환경을 올바르게 설정하기 전에 작성했던 코드로, 해당 구문을 삭제해도 테스트가 정상적으로 수행되는 것을 확인했습니다.

setup-globals.js는 위 설정 만을 수행 하므로 파일 및 설정 삭제하도록 하겠습니다.

js87zz · 2021-06-09T05:42:37Z

test/customEvents.spec.js

+      spy = function() {};
+      spy2 = function() {};


스파이가 아니라 빈 함수를 할당한 이유가 있나요~?

jest.fn() 으로 생성되는 mock 함수는 jasmine.createSpy()로 생성되는 mock 함수와 다르게 Function을 상속받지 않습니다.

// customEvents.spec.js 194 it('by handler function.', function() { 195 ce.on('play', spy); 196 ce.off(spy); 197 198 expect(ce.events).toEqual({'play': []}); 199 });

위 테스트의 경우 ce.off(spy)를 수행하면서 주어진 인자가 어떤 유형(이벤트 명 or 핸들러 함수 or 객체) 인지 확인하고 이에 따라 이벤트 등록을 해제하는 작업을 수행합니다.
이때, spy에 jest mock 함수를 할당하면 이를 함수가 아닌 객체로 인지하고 작업을 수행해 의도치 않은 결과가 발생합니다.

Suggested change

spy = function() {};

spy2 = function() {};

var spyMock = jest.fn();

var spyMock2 = jest.fn();

spy = function() {

spyMock();

};

spy2 = function() {

spyMock2();

};

위와 같이 사용하면 mock 함수도 사용하고 동작도 정상적으로 되는 것을 확인할 수 있습니다.
그러나 spy와 spy2를 핸들러 함수로 사용하는 테스트는 핸들어의 수행 여부 등을 확인하지 않고 단순히 등록, 해제가 정상적으로 이뤄지는지 확인합니다.
그래서 굳이 위 처럼 mock 함수를 사용할 필요가 없다고 판단해서 위처럼 빈 함수를 할당해서 사용했습니다.

혹시 mock 함수로 변경해서 동작하도록 수정을 할까요?

음 뭔가 CustomEvent 내부에서 함수 여부를 체크하는 로직이 있어서 제대로 실행이 안되나 보네요. 변경된 방법이 제대로 검증할 수 없다면 유지하는게 좋을 것 같습니다.

넵! 그러면 지금은 현재 테스트 코드를 유지하도록 하겠습니다.
추후 좀 더 좋은 방법을 찾게되면 해당 방법을 적용해보도록 하겠습니다.

jest.fn 을 사용하면서 임의의 mock 함수가 함수 타입을 가지려고 한다면, 다음과 같이 mockImplementation API를 활용하면 될 것 같습니다.

const spyMock = jest.fn().mockImplementation(() => {});

실제 구동 예제는 여기서 확인해보실 수 있습니다.
https://codesandbox.io/s/objective-hill-b6j80?file=/src/index.test.js

구동 예제의 테스트를 실제 환경에서 실행보니 마찬가지로 Function을 상속받지 않는 것을 확인했습니다.
또한 정확히 어떤 이유에서 다른 동작을 보이는 건지는 알 수 없습니다만, 구동 예제의 환경에서는 jest.fn()으로 생성되는 mock 함수도 Function을 상속 받는 것으로 보입니다.

const spy = jest.fn(); Object.setPrototypeOf(spy, Function); console.log(spy instanceof Function); // true

위 처럼 mock 함수의 프로토타입 체인에 Function을 강제로 추가해주면 의도한대로 동작합니다.
그러나 이렇게 mock 함수에 임의의 변형을 가하는게 괜찮은가에 대한 의문이 조금 남아있어 현재 관련 내용을 찾아보는 중 입니다!

제 생각에 더 이상 이 부분을 어떻게 jest에 맞게 변형해서 테스트를 통과시킬까에 대해 시간 쏟을 필요는 없어 보여요. 이건 도구의 동작 방식일뿐이고 그를 인지 했다면, 최대한 가독성 있는 테스트로 작성할 수 있으면 될 것 같아요. 지금 테스트 코드 자체가 도구에 맞춰서 변경되다 보니 가독성이 점점 떨어지는 것 같기도 하구요.
제가 이후에 코드를 자세히 보진 않아서 자세히는 모르겠지만, 최초 작성한 테스트코드가 더 보기 편할 것 같아요(제 예상입니다ㅎㅎ)
그리고 ce.events와 같은 내부 프로퍼티를 직접 검증하는 테스트가 꼭 필요한 것인지도 다시 생각해보면 좋을 것 같습니다. 얼핏 다른 테스트를 봤는데 좀 비슷한 경우가 있는 것 같아 중복되지 않는지도 한번 확인해보면 좋겠어요.
앞의 조건에 해당하지 않고 꼭 필요한 테스트라면 더 이상 jest에 맞춰 변경하지 않고 테스트 코드의 가독성에 맞춰 변경하거나 유지하는게 좋을 것 같습니다~

js87zz · 2021-06-09T05:45:42Z

test/customEvents.spec.js

      ce.on('a  b  c', handler);

-      expect(ce.events).toEqual(jasmine.objectContaining({
+      expect(ce.events).toEqual(expect.objectContaining({


toMatchObject 매처로 한번에 검증할 수 있지 않을까요?

위 matcher 사용하는 것으로 변경하겠습니다!

js87zz · 2021-06-09T05:48:03Z

test/domutil.spec.js

                '<rect class="origin" id="rect" x="10" y="10" width="50" height="50"></rect>' +
-            '</svg>'
-    );
+            '</svg>';


나중에 여력이 되면 es6문법으로 전반적인 코드 변경 고려해도 좋을 것 같아요

js87zz · 2021-06-09T05:54:18Z

test/tricks.spec.js

+      window.setTimeout.mock.calls[0][0]();
      expect(spy).toHaveBeenCalled();


이 과정이 잘 이해가 안가는데 debounce가 호출되면 spy가 호출되었는지 검증할 수 있지 않나요? 타이머 실행 문제라면 https://jestjs.io/docs/timer-mocks 참조해도 좋을 것 같아요

아 제가 debounce의 동작을 이상하게 이해해서 저런 이상한 코드가 나왔습니다....
timer mock 사용하는 방법으로 수정하겠습니다!

js87zz · 2021-06-09T05:55:34Z

test/tricks.spec.js

+      spy = function() {
+        var args = Array.prototype.slice.call(arguments);
+        mockFunc.apply(null, args);
+      };


이 스파이는 어떤 용도일까요?

이 스파이는 제가 debounce 테스트에서 setTimeout을 mocking 할 때 테스트 간 의존이 있다는 것을 파악하지 못하고 다른 원인에 의해서 그런것 이라 생각해서 위 처럼 스파이를 설정 했었습니다.(실제로 스파이를 위처럼 설정 후 2개의 테스트가 통과 했었습니다...)

Suggested change

spy = function() {

var args = Array.prototype.slice.call(arguments);

mockFunc.apply(null, args);

};

spy = jest.fn();

현재는 테스트간 의존성을 해결한 상태이기 때문에 이처럼 작성할 필요가 없기 때문에 위 처럼 수정하도록 하겠습니다.

js87zz

리뷰완료입니다. 고생하셨습니다!

lja1018

리뷰완료합니다. 고생하셨습니다!

lja1018 · 2021-06-09T06:20:37Z

test/ajax.spec.js

+      result = '';
+      keys = Object.keys(this.responseHeaders);
+
+      for (i = 0; i < keys.length; i += 1) {


key.length도 변수에 담아서 사용하면 좋을 것 같아요.

Suggested change

for (i = 0; i < keys.length; i += 1) {

var length = keys.length;

for (i = 0; i < length; i += 1) {

위처럼 변수에 담아 사용하는 것으로 변경하겠습니다.

lja1018 · 2021-06-09T06:22:06Z

test/ajax.spec.js

+  },
+  install: function() {
+    var xhr;
+    this.xhr = xhr = {


this.xhr = xhr은 밑에서 따로 수행하는게 어떤가요?

지역변수 xhr에 먼저 할당 후 this.xhr = xhr;를 수행하는 것으로 변경 하겠습니다.

lja1018 · 2021-06-09T06:31:21Z

test/domevent.spec.js

  beforeEach(function() {
-    fixture.set('<button id="test">test</button>' +
-                    '<div id="abs" style="position:absolute;left:10px:top:10px">a</div>');
+    document.body.innerHTML = '<button id="test">test</button><div id="abs" style="position:absolute;left:10px:top:10px">a</div>';


left: 10px:이네요 :) ;이 되야합니다

chore: change to a more appropriate matcher

jajugoguma added 2 commits June 8, 2021 21:35

chore: set jest test environment

42d324f

chore: remove karma-jasmine test environment

chore: migrate tests from jasmine to jest

cdbbff8

jajugoguma requested review from heenakwag, lja1018 and shiren June 8, 2021 12:50

jung-han reviewed Jun 9, 2021

View reviewed changes

js87zz reviewed Jun 9, 2021

View reviewed changes

js87zz approved these changes Jun 9, 2021

View reviewed changes

lja1018 reviewed Jun 9, 2021

View reviewed changes

jajugoguma added 6 commits June 9, 2021 16:40

chore: apply code reviews

8ef82c1

chore: update readme

e6e4f9e

chore: change mocking method

73c36ca

chore: improve readability of tests

a2a0710

chore: change to a more appropriate matcher

chore: apply code reviews

0d3639c

docs: change the word to match contextual intent

02d0cbb

jajugoguma merged commit 0fe4954 into master Jun 18, 2021

jajugoguma deleted the chore/karma-to-jest branch June 18, 2021 18:23

		@@ -0,0 +1 @@
		{ "typeAcquisition": { "include": [ "jest" ] } } No newline at end of file

		@@ -0,0 +1,2 @@
		// eslint-disable-next-line strict
		global.Element = Element;

		window.setTimeout.mock.calls[0][0]();
		expect(spy).toHaveBeenCalled();

	for (i = 0; i < keys.length; i += 1) {
	var length = keys.length;
	for (i = 0; i < length; i += 1) {

chore: migrate tests from karma to jest #68

chore: migrate tests from karma to jest #68

Uh oh!

Conversation

jajugoguma commented Jun 8, 2021

Please check if the PR fulfills these requirements

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jung-han Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js87zz Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js87zz left a comment

Choose a reason for hiding this comment

Uh oh!

lja1018 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

jung-han Jun 9, 2021 •

edited

Loading

js87zz Jun 10, 2021 •

edited

Loading