Feat/example page test #366

jinwoo-kim-nhn · 2020-03-23T03:57:17Z

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

(selenium-webdriver)로 예제페이지 테스트 하는코드 추가.
- 예제페이지가 로드되면 window.errorLogs를 확인하여 에러가 있으면 로그를 찍고 테스트를 실패시킴
- 예제페이지 목록은 tuidoc.config.json 을 확인하여 경로로부터 자동으로 가져오도록 함
github actions 워크플로우에서 오전 7시마다 테스트하도록 함

jung-han · 2020-03-24T06:07:43Z

package.json

@@ -59,11 +59,12 @@
    "mkdirp": "^0.5.1",
    "optimize-css-assets-webpack-plugin": "^5.0.3",
    "safe-umd-webpack-plugin": "^4.0.0",
+    "selenium-webdriver": "^4.0.0-alpha.7",


selenium webdriver 버전도 같이 올라가야 하나요?

selenium-webdriver 는 기존에 사용안하다가 새로 추가되는 모듈입니다.ㅎㅎ

jung-han · 2020-03-24T06:14:26Z

exampleTest.js

+    const driver = getDriver(index);
+    const result = [];
+
+    for(let i = 0; i <= urls.length - 1; i++) {


for(let i = 0; i < urls.length; i += 1) {도 동일한 의미의 구문일 것 같습니다! 추가적인 연산이 들어간 것 같습니다.

넵 그렇네요. 반영하겠습니다. 감사해요

jung-han · 2020-03-24T06:16:59Z

exampleTest.js

+const BROWSERSTACK_USERNAME = process.env.BROWSERSTACK_USERNAME;
+const BROWSERSTACK_ACCESS_KEY = process.env.BROWSERSTACK_ACCESS_KEY;


const { BROWSERSTACK_USERNAME, BROWSERSTACK_ACCESS_KEY } = process.env;

로 디스트럭처링 해서 작성할 수 있을 것 같습니다.

넵 맞네요 ㅎㅎ 반영하겠습니다. 감사합니다.

jung-han · 2020-03-24T06:22:33Z

exampleTest.js

+        await driver.wait(function() {
+          return driver.executeScript('return document.readyState').then(function(readyState) {
+            return readyState === 'complete';
+          });
+        }, 20000);


await driver.wait(() => driver.executeScript('return document.readyState').then(readyState => readyState === 'complete') , 20000);

로 줄일 수 있겠네요!

맞네요. 반영하겠습니다.

20000은 어떤 의미인가요?정확한 기준이나 가이드가 있는건가요?

조건문이 만족할때 까지 잠시 스레드를 멈춘다고 합니다. 20000 은 최대시간 20초를 말하고, 20초 전에라도 조건이 완료되면 계속 실행되기때문에 속도에는 큰 문제가 없을듯 합니다. 문서에 기본값에 대한 정확한 명시가 없어서 안전하게 20000ms로 임의로 넣었습니다. .

@jinwoo-kim-nhn
넵 감사합니다~! 저 숫자는 그럼 상수화시켜서 의미가 명확하면 더 좋을 것 같습니다!아니면 주석으로 간단한 설명이 있어도 좋을 것 같습니다.

js87zz · 2020-03-25T01:38:04Z

exampleTest.js

+/**
+ * Url test
+ */
+async function urlTest(urls) {


함수 이름이 urlTest라서 뭔가 url자체를 테스트하는 느낌인대요. test또는 testExamplePage는 어떨까요?

넵 좋네요. 반영하겠습니다.

js87zz · 2020-03-25T01:51:21Z

exampleTest.js

+    const config = require(path.resolve(process.cwd(), 'tuidoc.config.json'));
+    const filePath = (config.examples || {filePath: ''}).filePath;
+    return fs.readdirSync(filePath).reduce((urls, fileName) => {
+        if (fileName.match(/html$/)) {


반환된 정보를 활용하지 않기때문에 이 경우는 정규식 test를 사용하는게 더 적합할 것 같습니다!

넵 거기까진 생각하지 못했네요, 반영하겠습니다.

js87zz · 2020-03-25T01:55:10Z

exampleTest.js

+function getDriver(index) {
+    return new Builder()
+        .usingHttpAgent(HttpAgent)
+        .withCapabilities(Object.assign({}, capabilities[index], {build: `examplePageTest-${new Date().toLocaleDateString()}`}))


스프레드가 조금 더 보기 편하지않을까요?

{ ...capabilities[index], build: `examplePageTest-${new Date().toLocaleDateString()}` }

넵 맞네요. 반영하겠습니다 감사합니다 ㅎㅎ

js87zz · 2020-03-25T02:14:29Z

exampleTest.js

+    const parallelPendingTests = Object.keys(capabilities).map(index => testPlatform(index, urls));
+    const testResults = await Promise.all(parallelPendingTests);
+    const result = testResults.flat().reduce((errorList, errorInfo) => {
+        if (!errorInfo.errorLogs) {


testPlatform 로직을 보면 errorLogs가 없으면 아무 에러가 안 난 상황일 것 같은데 처리해주는 이유가 있을까요?

예제페이지에 errorLogs에 관한 코드스닙펫이 추가 안되어있을 경우나 페이지를 제대로 get 해오지 못할경우를 생각하여 처리하였습니다. 정상적인 페이지인 경우 배열이 리턴되기때문에 그렇고. 구두로 논의한대로 더 알아보기 쉽게 수정해보겠습니다.

js87zz · 2020-03-25T02:19:02Z

exampleTest.js

+ * Url test
+ */
+async function urlTest(urls) {
+    const parallelPendingTests = Object.keys(capabilities).map(index => testPlatform(index, urls));


여기서 index를 넘기지 않고 배열의 엘리먼트를 바로 넘기는게 낫지 않을까요?getDriver에서도 capabilities[index]로 index를 사용해서 따로 빼내지 않아도 될 것 같구요.

네 이건 저도 인지하고 있었는데. 못고쳤네요 반영하겠습니다.!

js87zz · 2020-03-25T02:21:08Z

exampleTest.js

+async function urlTest(urls) {
+    const parallelPendingTests = Object.keys(capabilities).map(index => testPlatform(index, urls));
+    const testResults = await Promise.all(parallelPendingTests);
+    const result = testResults.flat().reduce((errorList, errorInfo) => {


testResults의 내용이 error가 나지 않은 경우에도 정보가 담겨있는데 errorInfo보다는 다른 네이밍이 좋지 않을까요?

넵 좀더 고민해서 고쳐보겠습니다 ㅎㅎ

js87zz · 2020-03-25T02:24:23Z

exampleTest.js

+        console.log(errorInfo.url);
+        console.log(errorInfo.browserName, errorInfo.browserVersion, errorInfo.errorLogs);


디스트럭쳐링으로 쓰면 좀 더 간결하게 줄일수 있을 것 같아요~!그리고 error log는 chalk 같은 라이브러리 사용해서 로그에 색깔이 구분되도록 할 수도 있을까요?그러면 더 눈에띄고 보기 좋을 것 같아서요.

넵 디스트럭처링 반영하겠습니다. chalk는 최대한 디펜던시를 만들고 싶지 않아서.. 고려해보겠습니다.

아 의존성 문제가 있군요. 우선은 고려안하는게 좋겠네요!ㅎㅎ

js87zz · 2020-03-25T02:26:58Z

exampleTest.js

+
+    printErrorLog(result);
+
+    assert.equal(result.length, 0);


이 단언문은 에러가 있을 경우 단언에 실패하도록 추가된건가요?

네 에러가 있으면 실패되도록 처리하기위해 사용했습니다.

에러 객체를 throw 하는 건 어떨까요?그러면 위의 catch 하는 쪽에서도 메시지가 더 명시적으로 나오게 만들수 있을 것 같아서요.

assert가 깨질경우 알아서 에러를 던지므로 따로 throw 문을 쓰지 않아도 상관없을꺼 같아요.

* feat: example test * feat: added action workflow * push test * restore * test 2 * commit for fail test * test 3 * test 4 * error test 5 * success test 1 * builder open close improvements * fail Tests with improved speed. * feat: make test target urls * update workflow for exampletest * some refactoring * remove unnessary script * apply codereview - 1 * apply codereview - 2 Co-authored-by: superlucky84 <[email protected]>

jinwoo-kim-nhn added 16 commits March 19, 2020 17:42

feat: example test

ca3ad5d

feat: added action workflow

d00d746

push test

cc0eba1

restore

f593f52

test 2

5292f2d

commit for fail test

0d397d1

test 3

18b8a93

test 4

099651e

error test 5

309e529

success test 1

202f45b

builder open close improvements

6294fb5

fail Tests with improved speed.

316bee6

feat: make test target urls

416733b

update workflow for exampletest

023695f

some refactoring

15a2493

remove unnessary script

e453e95

jung-han reviewed Mar 24, 2020

View reviewed changes

js87zz reviewed Mar 25, 2020

View reviewed changes

superlucky84 and others added 2 commits March 25, 2020 19:29

apply codereview - 1

25cbdc0

apply codereview - 2

74d87d2

Merge branch 'master' into feat/examplePageTest

56acc3e

jinwoo-kim-nhn merged commit 3b02631 into master Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/example page test #366

Feat/example page test #366

jinwoo-kim-nhn commented Mar 23, 2020 •

edited

Loading

jung-han Mar 24, 2020

jinwoo-kim-nhn Mar 24, 2020

jung-han Mar 24, 2020

jinwoo-kim-nhn Mar 24, 2020

jung-han Mar 24, 2020

jinwoo-kim-nhn Mar 24, 2020

jung-han Mar 24, 2020

jinwoo-kim-nhn Mar 24, 2020

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020

js87zz Mar 25, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020

js87zz Mar 25, 2020

jinwoo-kim-nhn Mar 25, 2020

		const BROWSERSTACK_USERNAME = process.env.BROWSERSTACK_USERNAME;
		const BROWSERSTACK_ACCESS_KEY = process.env.BROWSERSTACK_ACCESS_KEY;

		console.log(errorInfo.url);
		console.log(errorInfo.browserName, errorInfo.browserVersion, errorInfo.errorLogs);

Feat/example page test #366

Feat/example page test #366

Conversation

jinwoo-kim-nhn commented Mar 23, 2020 • edited Loading

Please check if the PR fulfills these requirements

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

js87zz Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

js87zz Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js87zz Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js87zz Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js87zz Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinwoo-kim-nhn commented Mar 23, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

jinwoo-kim-nhn Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading

js87zz Mar 25, 2020 •

edited

Loading