RankChartの`utils.js`内の処理のテストを追加 #868

ryoppippi · 2025-02-05T15:43:02Z

昨日のリファクタリングが合ってるか確証が取れなかったのでテストを追加

vitestの導入
昨日の変更を全てrevert
テストを追加
変更のreapply
新しい関数に合わせてテストを修正&追加。出力が変わらないことを確認

This reverts commit 4fa5cde.

This reverts commit 662a257.

This reverts commit 5b8bb1c.

This reverts commit defb457.

This reverts commit 14589b7.

This reverts commit 18c04c0.

This reverts commit 1c58577.

This reverts commit fa0cd17.

This reverts commit 97e35c5.

This reverts commit bebd5aa.

Add comment and limit test data to first 50 articles to prevent test flakiness from varying dataset size.

ryoppippi · 2025-02-05T15:51:40Z

お、vitestを導入するかどうかって話が出てたのか

#743 (comment)

まあ、Astroが推奨してるしいいのではという感想ですが、いかがでしょう。
実際ここ最近の風潮を見ているとvitest, node test, Bun testの3択ですが、AstroがViteベースなので素直にvitest使っておくのが無難だと思われますね
https://docs.astro.build/en/guides/testing/

tomoya · 2025-02-05T16:08:02Z

src/components/RankChart/utils.test.ts

+  beforeEach(() => {
+    /** テストの結果が変わらないように、最初の50記事のみを取得 */
+    const PUBLISHED_FIRST_50_ARTICLES = getArticles({
+      isPublished: true,
+    }).slice(0, 50);
+    RANKING = utils.getRanking(PUBLISHED_FIRST_50_ARTICLES);
+    TOP_10_RANKING = utils.getTopNRanking(RANKING, 10);
+    Object.freeze(TOP_10_RANKING); // Ensure that the array is not mutated
+  });
+
+  it("getRanking", () => {
+    expect(RANKING).toMatchSnapshot(); // Snapshotが大きいので別途ファイルに保存
+  });


このテストのやり方だと、今後ロジックへの変更が行われた時に、スナップショットを更新するだけになるので、こういったケースではスナップショットテストは向いていないと思います。
とりあえず getRanking の実装コードにあるコメントの動作を確認できる最小のデータとケースを作ってテストするのが良いと思いました。

とりあえず getRanking だけテストして、他は別になくてもいいんじゃないかなという気がしますね。

なるほど、snapshotじゃなくて

ちゃんとソートされているか

userのランクが適切かどうか
みたいなものを個別にテストした方がいいってことですね

ちなみに他の関数についてもsnapshotじゃない方がいいんでしょうか

なるほど、とりあえずgetRankingのテスト書き直します

42b0dd4
ロジックを一つずつテストに落としてみました

他もsnapshot使うのは適切でないからごそっと消すべき？

getData と getOptions はたいしたロジックもないので snapshot でいいんじゃないかなという印象ですね。
ただ、describe ブロックは分けたほうがいいと思います。
特に beforeEach で初期化したデータをすべての関数のテストで利用しているのは、変化にとても弱いのでよくないです。

なるほど、一つ一つ分けてロジック書きますか

`getRanking`にコメントとして結果の変数が満たしうる条件を残しているので、それらが満たされているかどうかをチェック

tomoya · 2025-02-05T17:19:21Z

src/components/RankChart/utils.test.ts

+    /** ランクが1から始まっているか */
+    expect(ranking[0].rank).toBe(1);
+
+    /** 同じ記事数の場合は同じランクになるか.ソートされているか. */
+    Array.from({ length: 50 }).forEach((_, i) => {
+      const current = ranking[i];
+      const prev = ranking[i - 1];
+
+      /** どちらかが存在しない場合はスキップ */
+      if (!(current && prev)) {
+        return;
+      }
+
+      if (prev && prev.articleCount === current.articleCount) {
+        expect(current.rank).toBe(prev.rank);
+      } else {
+        expect(current.rank).toBeGreaterThan(prev.rank);
+      }
+    });
+
+    /** 記事数の合計が50であるか */
+    const totalArticleCount = ranking.reduce(
+      (acc, { articleCount }) => acc + articleCount,
+      0,
+    );
+
+    expect(totalArticleCount).toBe(50);
+  });


同じ it ですべてテストせずに、 expect の数だけ it 分けたほうがテストケースの目的が単一になってよいです。

なるほど、getRanking単体でdescribeを作ってitで分けるべきですね

tomoya · 2025-02-05T17:20:54Z

src/components/RankChart/utils.test.ts

+    Array.from({ length: 50 }).forEach((_, i) => {
+      const current = ranking[i];
+      const prev = ranking[i - 1];
+
+      /** どちらかが存在しない場合はスキップ */
+      if (!(current && prev)) {
+        return;
+      }
+
+      if (prev && prev.articleCount === current.articleCount) {
+        expect(current.rank).toBe(prev.rank);
+      } else {
+        expect(current.rank).toBeGreaterThan(prev.rank);
+      }
+    });


テストの中でループと条件分岐はバッドブラクティスです。

この場合、どうすればいいんでしょう。
条件分岐なしにテストを書くのが難しそうなので気になりました

わかりました。具体的なもので作り直します

ケースにそった input を用意して expect(getRanking(input)).toEqual(expected) のようにテストすれば十分だと思います。
この場合は、 同じ記事数の場合は同じランクになる と ソートされている の2種類のinputを用意すれば大丈夫なはずです。

ryoppippi · 2025-02-05T17:55:42Z

@tomoya 色々直しました！

ダミーを使う
結果をベタ書きする

tomoya · 2025-02-06T05:54:13Z

src/components/RankChart/utils.test.ts

+const ARTICLES = [
+  { githubUser: "alice" },
+  { githubUser: "alice" },
+  { githubUser: "alice" },
+  { githubUser: "bob" },
+  { githubUser: "bob" },
+  { githubUser: "bob" },
+  { githubUser: "charlie" },
+  { githubUser: "charlie" },
+  { githubUser: "david" },
+];


だいぶいい感じになってきましたね。
それぞれのテストケースが何を目的としているのかわかるようになってきて、可読性も変更耐性も上がってきた感じがします。

次は、テストデータがこの ARTICLES ひとつですべてをまかなっているのを変えましょう。
いまだと、何か機能を追加したりするとき、テストデータを変更しなければ実現できない場合、このARTICLES の値を変更すると、その機能追加とは関係のないテストケースも失敗するようになるはずです（例：数が変わるといくつかのテストが落ちる）。

ですので、基本的にはそれぞれのテツトケースでテストデータを用意するようにしましょう。その際に、必要最小限のテストデータにすることで、データからテストの意図が読み取れるようになり、より可読性が向上します。

具体的には最初のテストのこのコードは、

const ranking = utils.getRanking(ARTICLES); it("should start with rank 1", () => { expect(ranking[0].rank).toBe(1); });

こんな感じで、テストデータの順番から正しくランク順に並んでいることが示されればよい（数の重複は別のテストケースで担保するので、このテストではそういうデータを用いない）というわけです。

it("should start with rank 1", () => { expect(utils.getRanking([ { githubUser: "alice" }, { githubUser: "bob" }, { githubUser: "bob" }, { githubUser: "bob" }, { githubUser: "charlie" }, { githubUser: "charlie" }, ])).toEqual([ { rank: 1, user: "bob", articleCount: 3, }, { rank: 2, user: "charlie", articleCount: 2, }, { rank: 3, user: "alice", articleCount: 1, } ]); });

考え方は最小構成と同じで、再現に必要なもの以外をすべて排除すること。

修正しました！レビューお願いします！

vim-jp#868 (comment)

tomoya · 2025-02-06T13:00:40Z

これで各テスト同士の依存がなくなったので、次はテストの質を改善するフェーズですかね。

tomoya · 2025-02-06T13:03:26Z

src/components/RankChart/utils.test.ts

+    it("should start with rank 1", () => {
+      const ranking = utils.getRanking([
+        { githubUser: "alice" },
+        { githubUser: "bob" },
+        { githubUser: "bob" },
+        { githubUser: "bob" },
+        { githubUser: "charlie" },
+        { githubUser: "charlie" },
+      ]);
+      expect(ranking[0].rank).toBe(1);
+    });


このテストだと、配列の1件目が rank 1 にはなってるけど、user などの値はが何でも構わないというテストになるわけですが、それが本当に担保するべき仕様なのでしょうか？
ちなみに、そうであるならば、テストデータの配列は1件でも十分になります。

ユーザーの値はなんでもいいですね。なので、一件でも十分だと思われます

tomoya · 2025-02-06T13:11:52Z

src/components/RankChart/utils.test.ts

+    it("should have correct ranking", () => {
+      const ranking = utils.getRanking([
+        { githubUser: "alice" },
+        { githubUser: "alice" },
+        { githubUser: "alice" },
+        { githubUser: "bob" },
+        { githubUser: "bob" },
+        { githubUser: "bob" },
+        { githubUser: "charlie" },
+        { githubUser: "charlie" },
+        { githubUser: "david" },
+      ]);
+      expect(ranking[0]).toEqual({
+        rank: 1,
+        user: "alice",
+        articleCount: 3,
+      });
+      expect(ranking[1]).toEqual({
+        rank: 1,
+        user: "bob",
+        articleCount: 3,
+      });
+      expect(ranking[0].rank).toBe(ranking[1].rank);
+
+      expect(ranking[2]).toEqual({
+        rank: 3,
+        user: "charlie",
+        articleCount: 2,
+      });
+
+      expect(ranking[3]).toEqual({
+        rank: 4,
+        user: "david",
+        articleCount: 1,
+      });
+    });


correctとか、日本語だとよく「正しい」とか書いたりしますが、何が正しいのかが読んでも理解できないので、テストケースの説明としては不十分になります。

あと、１回の実行結果の expect を複数に分ける必要はなくて、配列ぜんぶを比較すればいいはずです。ぜんぶ比較しないのであれば、その理由が欲しいです。

そうですね、一つのexpectにするのが適切ですね

vim-jp#868 (comment) - 説明に具体的な動作を追加 - 複数のexpectを一つにまとめ、配列全体で比較

vim-jp#868 (comment)

ryoppippi · 2025-02-06T13:29:07Z

修正しました！

お願いします！

ryoppippi · 2025-02-06T13:30:25Z

src/components/RankChart/utils.ts

@@ -4,7 +4,7 @@ import type { Article, Ranking } from "./types";
 /**
 * ランキングを計算し、1つの配列にまとめる
 */
-export function getRanking(articles: Article[]): Ranking[] {
+export function getRanking(articles: Pick<Article, "githubUser">[]): Ranking[] {


テストを書くと、必要なのは githubUserだけだとわかるので、余計な引数を要求しなくていいことがわかりましたね。

tomoya · 2025-02-06T13:32:58Z

src/components/RankChart/utils.test.ts

+      expect(ranking[0].rank).toBe(1);
+    });
+
+    it("should assign same rank for ties and skip next ranks appropriately", () => {


同順位がない場合のテストケースもあった方がいいと思いました。
そうすれば、仮にどう順位のロジックに変更があったとしても、そうじゃない場合は変わっていないということが担保されるためです。

ちなみに今の同順位の仕様としては、引数配列の登場順という仕様が暗黙としてあり、名前順に変えるなどの変更を加えようとした時に、2つのテストケースがいい感じに機能します。

ちなみに登場順に並んでいる仕様に関してはテストした方がいいんでしょうか
例えば

const articles = [ { githubUser: "alice" }, { githubUser: "alice" }, { githubUser: "bob" }, { githubUser: "bob" }, ];

と

const articles = [ { githubUser: "bob" }, { githubUser: "bob" }, { githubUser: "alice" }, { githubUser: "alice" }, ];

の２パターン作るとか

https://github.com/vim-jp/ekiden/pull/868/files/0d8988242e762b21889606e38a2fd7ce2155a13d#r1944733777

ryoppippi · 2025-02-06T14:00:20Z

重複あるなしを分けてみました

tomoya

これで良さそうですね！

ryoppippi · 2025-02-06T16:24:51Z

ありがとうございます！
他の皆さんもマージして良さそうですか？

thinca

細かいことを言うとコミットログがだいぶとっ散らかってしまっているなと思いましたが、細けぇこたぁいいんだよ、というのもあるので Approve しておきます。

以下は私見なので別の意見もあるかも。

Revert はマージコミットに対してやればよい
レビューの指摘とその修正をいちいちコミットに残す必要はない。後から見てわかりやすいか、Revert しやすいかの単位になっているかの方が重要

ryoppippi · 2025-02-06T16:36:13Z

マージコミットに対してrevertは本当にそうですね

指摘をコミットに残しておくと後から振り返りやすいと思ったのですがどうなんでしょう

もはや汚すぎてsquash mergeしたい

ryoppippi · 2025-02-06T17:03:12Z

とりあえずsquash mergeします

ryoppippi added 7 commits February 5, 2025 15:13

chore(deps): add vitest

4c9a959

feat(ci): add test

9a8ec37

Revert "fix(RankChart): acc.at -> acc[ ]"

bebd5aa

This reverts commit 4fa5cde.

Revert "refactor(RankChart): ランキング取得関数のreduce部の現在のユーザーの変数名を変更"

97e35c5

This reverts commit 662a257.

Revert "fix(RankChart): top 10までのランキングを取得する処理を別の関数に切り出す"

fa0cd17

This reverts commit 5b8bb1c.

Revert "fix(RankChart): 上位10名を選択するlogicをsvelte側に移動"

1c58577

This reverts commit defb457.

Revert "fix(RankChart): ランキングの計算ロジックをモダンな記法に変更"

18c04c0

This reverts commit 14589b7.

ryoppippi requested review from thinca, monaqa, kyoh86, peacock0803sz and staticWagomU as code owners February 5, 2025 15:43

ryoppippi added 8 commits February 5, 2025 15:47

feat(RankCart): add test for the original process

5087b79

Reapply "fix(RankChart): ランキングの計算ロジックをモダンな記法に変更"

800b5dd

This reverts commit 18c04c0.

Reapply "fix(RankChart): 上位10名を選択するlogicをsvelte側に移動"

8ae435b

This reverts commit 1c58577.

Reapply "fix(RankChart): top 10までのランキングを取得する処理を別の関数に切り出す"

1cd4cc6

This reverts commit fa0cd17.

Reapply "refactor(RankChart): ランキング取得関数のreduce部の現在のユーザーの変数名を変更"

ffc9899

This reverts commit 97e35c5.

Reapply "fix(RankChart): acc.at -> acc[ ]"

568267c

This reverts commit bebd5aa.

test(RankChart): fix for top 10 rankings using getTopNRanking

d6465b2

test(RankChart): add comment for PUBLISHED_FIRST_50_ARTICLES

db3af87

Add comment and limit test data to first 50 articles to prevent test flakiness from varying dataset size.

ryoppippi force-pushed the feature/raking-test branch from c71dc41 to db3af87 Compare February 5, 2025 15:48

tomoya reviewed Feb 5, 2025

View reviewed changes

ryoppippi marked this pull request as draft February 5, 2025 16:13

test(RankChart): fix test for getRanking

42b0dd4

`getRanking`にコメントとして結果の変数が満たしうる条件を残しているので、それらが満たされているかどうかをチェック

ryoppippi force-pushed the feature/raking-test branch from 8321faa to 42b0dd4 Compare February 5, 2025 16:34

test(RankChart): 50件でsortされているか確認

ed978d8

ryoppippi marked this pull request as ready for review February 5, 2025 16:40

ryoppippi requested a review from tomoya February 5, 2025 16:40

tomoya reviewed Feb 5, 2025

View reviewed changes

ryoppippi added 2 commits February 5, 2025 17:43

fix(RankChart): getRankingの処理に必要な型だけを取り出して引数にする

f8aa1bc

test(RankChart): dummy dataを用意/関数ごとにdescribeを分ける/条件分岐を除く

137faf7

ryoppippi requested a review from tomoya February 5, 2025 17:55

tomoya reviewed Feb 6, 2025

View reviewed changes

test(RankChart): テストデータをテストごとに設置

9debcf6

vim-jp#868 (comment)

ryoppippi requested a review from tomoya February 6, 2025 12:20

tomoya reviewed Feb 6, 2025

View reviewed changes

test(RankChart): getRankingで同じarticleの時にrankが同じになるかのテストを修正

e66840b

vim-jp#868 (comment) - 説明に具体的な動作を追加 - 複数のexpectを一つにまとめ、配列全体で比較

ryoppippi added a commit to ryoppippi/ekiden that referenced this pull request Feb 6, 2025

test(RankChasrt): getRankingの配列の一件目がrank1になるテストに使用するデータを最小にする

1359a68

vim-jp#868 (comment)

ryoppippi requested a review from tomoya February 6, 2025 13:26

ryoppippi added 2 commits February 6, 2025 13:28

test(RankChart): getRankingの配列の一件目がrank1になるテストに使用するデータを最小にする

91cd76f

vim-jp#868 (comment)

test(RankChart): ユニークユーザの数を調べるテストの説明を修正

0d89882

ryoppippi force-pushed the feature/raking-test branch from 1359a68 to 0d89882 Compare February 6, 2025 13:28

ryoppippi commented Feb 6, 2025

View reviewed changes

tomoya reviewed Feb 6, 2025

View reviewed changes

test(RankChart): 同順位がある場合と無い場合のテストケースを分けて追加

d4adbbc

https://github.com/vim-jp/ekiden/pull/868/files/0d8988242e762b21889606e38a2fd7ce2155a13d#r1944733777

ryoppippi requested a review from tomoya February 6, 2025 14:00

tomoya approved these changes Feb 6, 2025

View reviewed changes

thinca approved these changes Feb 6, 2025

View reviewed changes

ryoppippi merged commit 8851758 into vim-jp:main Feb 6, 2025
2 checks passed

ryoppippi deleted the feature/raking-test branch February 6, 2025 17:03

ryoppippi restored the feature/raking-test branch February 6, 2025 17:03

vim-jp locked as resolved and limited conversation to collaborators Feb 8, 2025

RankChartのutils.js内の処理のテストを追加 #868

RankChartのutils.js内の処理のテストを追加 #868

Conversation

ryoppippi commented Feb 5, 2025

ryoppippi commented Feb 5, 2025 • 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

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

Choose a reason for hiding this comment

ryoppippi commented Feb 5, 2025

tomoya Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomoya commented Feb 6, 2025

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

ryoppippi commented Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoppippi Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

ryoppippi commented Feb 6, 2025

tomoya left a comment

Choose a reason for hiding this comment

ryoppippi commented Feb 6, 2025

thinca left a comment

Choose a reason for hiding this comment

ryoppippi commented Feb 6, 2025

ryoppippi commented Feb 6, 2025

RankChartの`utils.js`内の処理のテストを追加 #868

RankChartの`utils.js`内の処理のテストを追加 #868

ryoppippi commented Feb 5, 2025 •

edited

Loading

tomoya Feb 6, 2025 •

edited

Loading

ryoppippi commented Feb 6, 2025 •

edited

Loading

ryoppippi Feb 6, 2025 •

edited

Loading