test/routing #8
test/routing #8
Conversation
package.json
Outdated
| @@ -5,7 +5,7 @@ | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "start": "nodemon src/index.js", | |||
| "test": "mocha ./test/src/models" | |||
| "test": "mocha ./test/src/routes" | |||
There was a problem hiding this comment.
こうしてしまうと、modelsのテストがnpm test で実行できなくなります。
既存のテストもnpm test実行できるように修正お願いしますmm
There was a problem hiding this comment.
testディレクトリにmocha.optsを作成して、npm testでmodelsのテストも行えるようにしました。
test/src/routes/router_test.js
Outdated
|
|
||
| describe("GET /", () => { | ||
| it("top.ejsの表示", (done) => { | ||
| request(app) |
There was a problem hiding this comment.
GET / と GET /quiz で以下のコードが重複しているので、共通部分は1つの関数に抜き出すようにリファクタお願いします。
request(app)
.get("/")
.expect(200, done);その際に、getの引数に渡すURLやコールバック関数はリファクタ時に作成する関数の引数経由で渡すようにしてあげると良いでしょう。
There was a problem hiding this comment.
また、app.use("/api", quizRouter); に関するテストが見当たらないですが、apiのテストはまた別のプルリクで作成するという認識でよろしいでしょうか？
https://github.com/ha-matsumu/trivial-quiz-express/blob/test/routing/src/index.js#L12
|
コメントしたのでご確認お願いしますｍm |
|
コードの修正とapiのテストを作成しました。 |
test/src/routes/router_test.js
Outdated
|
|
||
| describe("GET /", () => { | ||
| it("top.ejsの表示", (done) => { | ||
| runRouteTest("/", done); |
There was a problem hiding this comment.
細かいですがインデントのスペースが1つになっているので、他と統一して2つにお願いしますmm
test/src/routes/router_test.js
Outdated
|
|
||
| describe("GET /quiz", () => { | ||
| it("quiz.ejsの表示", (done) => { | ||
| runRouteTest("/quiz", done); |
There was a problem hiding this comment.
test/src/routes/quizRouter_test.js がreturnを使ったPromiseスタイルでの非同期処理のテスト実装に対して、こちらのテストは done を使ったコールバックで非同期処理のテストを終了するスタイルにしている理由は何かありますでしょうか？
(テストの実装自体は問題ないのでこのままでもOKです^^ 統一されていない書き方が気になったので質問しました。)
There was a problem hiding this comment.
特に理由はないので、router_test.jsをPromiseスタイルに変更しました。
|
👍 実装自体問題ありません^^ |
|
修正しました。 |
test/src/routes/router_test.js
Outdated
|
|
||
| describe("GET /", () => { | ||
| it("top.ejsの表示", () => { | ||
| runRouteTest("/"); |
There was a problem hiding this comment.
Promise形式で実装するのであれば、itの中にreturnでPromiseオブジェクトをセットする必要があると思いますが、 returnなしで実装した場合runRouteTest("/"); を runRouteTest("/abc"); のように存在しないURLに変更した場合でもテストは失敗しますでしょうか？
ドキュメント抜粋
There was a problem hiding this comment.
コード
function runRouteTest(_url) {
request(app)
.get(_url)
.expect(200);
}
describe("GET /", () => {
it("top.ejsの表示", () => {
runRouteTest("/abc");
});
});
テスト実行結果
There was a problem hiding this comment.
/abc のルーティングは容易していないのでテストは失敗するべきですが、テストが通っているのはおかしい挙動となります。
/abc にしたまま、以下のようにitの中にreturnを使ったらテストは落ちるようになるかなと思いますがいかがでしょうか？
function runRouteTest(_url) {
request(app)
.get(_url)
.expect(200);
}
describe("GET /", () => {
it("top.ejsの表示", () => {
return runRouteTest("/abc");
});
});
テストでは、正しいURLを入力したときに意図した挙動をする正常系と、意図しない値をセットしたときにエラーや例外を発生する異常系の両方を合わせて容易しておくと、よりテストの安全性が保証されます。
There was a problem hiding this comment.
以下のようにコードを変更するとテストが落ちました。
function runRouteTest(_url) {
return request(app)
.get(_url)
.expect(200);
}
describe("GET /", () => {
it("top.ejsの表示", () => {
return runRouteTest("/abc");
});
});
describe("GET /quiz", () => {
it("quiz.ejsの表示", () => {
return runRouteTest("/quiz");
});
});
There was a problem hiding this comment.
以下の部分が意図したとおり200ではテストが失敗するようになっていますね^^
describe("GET /", () => {
it("top.ejsの表示", () => {
return runRouteTest("/abc");
});
});以下3つのパターンのテストを用意して全てのテストが通るようにしてください。(全てpassingになるようにする。(緑の✔だけ表示されるようにする))
- `GET /` → 成功 (ステータスコード200)
- `GET /quiz` → 成功 (ステータスコード200)
- `GET /abc` → 失敗 (ステータスコード404)
There was a problem hiding this comment.
3つのパターンのテスト通るように修正しました。
|
1点コメントしたのでご確認お願いしますmm |
|
コード修正しました。レビューよろしくお願いいたします。 |
|
👍 OKです^^ |
router.jsのテストを作成しました。レビューよろしくお願いいたします。