ただの精神的続編のライブラリを良くしていくためのステップを記録する
bun913「弊社内でも利用するかもしれないのに、元のリポジトリがパブリックアーカイブになっているのはまずいよなぁ・・・」という思いから単なるCloneとして開始したこちらのパッケージ。
利用者もそこまで多くないとは思いつつも、一歩一歩何かをよくしていく作業は割と好きだったりするので、せっかくだったらよくしていく過程を記録していこうと思いました。
途中で飽きたり、自分以外の人が何かをしてくれるかもしれませんが、それもOSSの良さなのでテキトーにやっていこう。
bun913MileStone を作った
まず大まかに考えていることをちゃんとGitHub のマイルストンとして作成した。
大まかにいうとしたいことはこの2つしかない。
- 大きい関数を分けたり、リファクタリングしたい
- JSをTSにコンバートしたい
ただ、これらをする前に「ちゃんと既存のパッケージの動作を変えていないよね」という心理的安全性を担保するものが必要。
この文脈ではテストですね。
ということで、したいことの前にちゃんと地道にやっていこうと思いました。
bun913まずは可視化・現状を知る
ブランチカバレッジを目安にするよ
既存のパッケージからのテストを引き継いで、ちょっとだけテストケースを足した状態のカバレッジは？
% Coverage report from v8
---------------------|---------|----------|---------|---------|----------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|----------------------
All files | 58.99 | 63.07 | 53.33 | 58.99 |
TestrailReporter.js | 54.08 | 60 | 75 | 54.08 | ...3,186-199,203-254
environment.js | 100 | 100 | 100 | 100 |
index.js | 0 | 0 | 0 | 0 | 1
logger.js | 64.28 | 41.66 | 100 | 64.28 | ...32-35,51-57,60-65
testRailApi.js | 55.17 | 72.72 | 45.45 | 55.17 | ...2-84,87-92,96-102
---------------------|---------|----------|---------|---------|----------------------
Codecovで嫌でも目に入るようにするよ
次にCodecovというサービスでカバレッジを可視化しておくことにしました。
私が以前取り組んでいたOSSでも使わせていただいていて、OSSなら無料で使えるのがありがたいです。
ということでこのようにカバレッジのバッジを表示するようにしました。
ここから特に重要かつ、リファクタリングのしがいがある TestRailReporter.js のブランチカバレッジを90%くらいまで引き上げていきたいところです。
必然的に分岐の境界値を丁寧に見ていくことで、「あぁ安心」度は上がっていくと想定しています。
bun913TestCase追加1
以下PRで実施。
TestrailReporter のカバレッジが上がった
bun913TestCase追加2
以下PRで実施。
今回はよくわからない TESTRAIL_STEPS という環境変数が与える結果を調べながらテストケースを追加した。
どうやら複数の同じテストケースIDに対するアサーションを一つの塊として捉えてくれるための機能らしい。デフォルトでOffだがデフォルトでOnで良くない？と思っていた。
TestRailReporter のカバレッジがこんな感じで上がった
やはりプログラムの基本の制御構造は分岐と繰り返し。
その中でも分岐を埋めていくことでかなり仕様理解につながっていくことを強く実感。
bun913ちなみにCodecovでこんな感じでテストケースが足りていない部分がわかるのが便利
テストのカバレッジだけみるのはよくないけど、今回の場合そもそもカバレッジが足りてない = 制御構造に対する挙動を見てない ということが明らかなので、とても便利に使える。
bun913TestCase追加3
なかなかテストがしにくい構造の関数のテストケース追加を始めた。
何が辛いって、本来やりたくない「振る舞い」ではなく「実装の詳細」に目を向けざるを得ない構造になっている。
ただしテストケースがまともにない状態でリファクタリングをするのは厳しい。
よってまずはちゃんとリファクタリングをするために、テストケースを追加していく・・・
やっとブランチカバレッジが80%を超えた。
80%を超えて、主要な関数を網羅できたらリファクタリングに着手していく予定（まずは関数を小さく）
bun913TestCase追加4
今日も地道に テストケースを追加した。
だいぶ挙動を把握してきた。
オプションが与える影響とかを。
bun913こんな感じで主要なロジックにはテストを追加できたと思う。
よって、次からみんな大好きリファクタリング編へ進みたい
---------------------|---------|----------|---------|---------|----------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|----------------------
All files | 80.16 | 81.73 | 55.88 | 80.16 |
TestrailReporter.js | 84.1 | 86 | 50 | 84.1 | ...3,181-184,194-203
environment.js | 100 | 100 | 100 | 100 |
index.js | 100 | 100 | 100 | 100 |
logger.js | 90 | 68.96 | 100 | 90 | 51-57
testRailApi.js | 59.48 | 80 | 50 | 59.48 | ...2-84,87-92,96-102
---------------------|---------|----------|---------|---------|----------------------
いまいちなテストケースもあるけど、テストを通しながらだいぶ理解が進んだ
✓ test/TestrailReporter.spec.js (26)
✓ TestrailReporter (26)
✓ onComplete (4)
✓ raise error when required options are missing (4)
✓ raise error when domain option is missing
✓ raise error when username option is missing
✓ raise error when apikey option is missing
✓ raise error when projectid option is missing
✓ jsonifyResults (13)
✓ converts newwman test results to TestRail-compatible JSON format (3)
✓ converts a test result to one TestRail test run
✓ converts two mapped test case to two TestRail testruns
✓ converts two test cases to two TestRail test runs
✓ failed test case handling (1)
✓ converts a failed test case to a failed TestRail test run
✓ skipped test case handling (1)
✓ converts a skipped test case to a skipped TestRail test run
✓ umarked test case handling (1)
✓ skip when not including test caseid top of the test case title
✓ TESTRAIL_STEPS env value is (3)
✓ true: reports multiple assertions as a steps if they have same test case ids (2)
✓ reports as an failed test case when one step fails, even if others succeed
✓ resports as an success test case when all steps are success
✓ false (default value): reports multipe assertions as multiple test results (1)
✓ reports as two test results
✓ User wants to establish connection by title not by case id (2)
✓ When TESTRAIL_TITLE_MATCHING env value is `true` (2)
✓ Newman assertions with title `TestTitle` matched to TestRail test case with `TestTitle`
✓ Newman assertions with title `NoMatchedTitle` not matched to TestRail test case with `TestTitle`
✓ test rail v1 API (1)
✓ works for TestRail v1 api
✓ custome env value (1)
✓ include a custom_key_value in result json when TESTRAIL_CUSTOM_$HOGE setted
✓ pushToTestRail (9)
✓ when results are available (2)
✓ uses provided title when title is set
✓ uses `${projectName}: Automated YYYY-MM-DD` formmated title when title is not set
✓ when runId is set (2)
✓ uses provided runId when runId environemnt variable is set
✓ get latest runId from Testrail when runId is equal to 'latest'
✓ when testPlan id is set (2)
✓ add new test-run under the project and use the tet-run id to add results
✓ runId is used if both testPlan and run-id are set
✓ when closeRun is set (2)
✓ close the run when closeRun is not set to 'false'
✓ does not close the run when closeRun is set to 'false'
✓ when no results are available (1)
✓ does not call any TestRail API
✓ test/lib/index.spec.js (1)
✓ TestRailReporter (1)
✓ smoke test (1)
✓ should be defined
✓ test/lib/logger.spec.js (6)
✓ Logger (6)
✓ getLogging (6)
✓ always url from the newman execution in the log
✓ includes query parameters in the url if present
✓ only returns url when TESTRAIL_LOGGING env variable is set to 'none'
✓ includes headers when TESTRAIL_LOGGING env variable is set to 'headers'
✓ includes full request and response when TESTRAIL_LOGGING env variable is set to 'full'
✓ output requestError when responseError is setted
✓ test/lib/testRailApi.spec.js (12)
✓ TestRailApi (12)
✓ constructor (2)
✓ adds X-API-IDENT header when betaApi is set to true
✓ does not add X-API-IDENT header when betaApi is set to false
✓ getProjectInfo (2)
✓ sends reqeust to DOMAIN/api/v2/get_project/testProjectId when suiteId is not set
✓ sends request to DOMAIN/api_v2_get_suite/:suiteId when suiteId is set
✓ getRuns (1)
✓ sends request to DOMAIN/api/v2/get_runs/:projectId when suiteId is not set
✓ getRun (1)
✓ sends request to DOMAIN/api/v2/get_run/:runId
✓ getCases (3)
✓ sends request to DOMAIN/api/v2/get_cases/:projectId when suiteId is not set
✓ sends request to DOMAIN/api/v2/get_cases/:projectId&suite_id=:suiteId when suiteId is set
✓ handles pagination when there are multiple pages of cases
✓ addRun (1)
✓ sends request to DOMAIN/api/v2/add_run/:projectId
✓ addPlanEntry (1)
✓ sends request to DOMAIN/api/v2/add_plan_entry/:planId
✓ closeRun (1)
✓ sends request to DOMAIN/api/v2/close_run/:runId
Test Files 4 passed (4)
Tests 45 passed (45)
Start at 00:27:13
Duration 370ms (transform 84ms, setup 0ms, collect 645ms, tests 33ms, environment 1ms, prepare 220ms)
リファクタリング開始
テストケースが揃ってきたので、安心してリファクタリングを開始した。
まずはこちらのPR
以下のように神クラス的になっているクラスから、責務を分離して挙動を少し明確にした。
「まずは着手しやすいところ」と思っていたが、これでも十分に面倒だった。
これから先の if文を削減していくのが楽しみだぜ！
bun913神的な関数をごっそり別のクラスに責務を以上することで非常に見通しをよくした。
関数を小さく分けたおかげで、分岐の検証や実際の振る舞い単位での検証が非常にしやすくなったのは本当に神としか言いようがない。実際神だと思う。
いや〜まさかLogというクラスが結果に影響を与えるとは思ってなかったので、テストケースを書くものですね。
bun913pushResultのリファクタリングをした。
こんにちは。尊いマンです。以下は尊さを表す画像です。
関数を別のクラスに出して、それなりの単位で関数を分割して、もちろんそれらのテストも追加しました。
