feat: Test implementation of update workflow #8150
Conversation
…ation-of-update-workflow
…ation-of-update-workflow
…ation-of-update-workflow
| expect(caller).rejects.toThrow('Cannot edit workflows that are not in progress'); | ||
| }); | ||
|
|
||
| it('Should fail when the target approverGroup does not exist', () => { |
There was a problem hiding this comment.
ここから 357 行目まで WorkflowApproverGroupService 内部の異常系のテストなので、WorkflowApproverGroup.integ.ts に切り出すのでも良いかと思いました。WorkflowApproverGroupService.removeApproverGroup 等 WorkflowApproverGroupService.updateApproverGroup から呼び出されている各メソッドはプライベートメソッドになっているため (対象クラスのパブリックなメソッドから呼ばれることが前提となっている)、いずれにせよインテグレーションテストで良いと判断しました。
There was a problem hiding this comment.
expect(caller).rejects.toThrow(message) の message 部分がちゃんとテスト対象と一致する .spec.ts や .integ.ts を作るべきなので、「WorkflowApproverGroup.integ.ts に切り出すのでも良い」ではなく「切り出さないといけない」
There was a problem hiding this comment.
WorkflowApproverGroupService.updateApproverGroup から呼び出されている各メソッドはプライベートメソッドになっているため (対象クラスのパブリックなメソッドから呼ばれることが前提となっている)、いずれにせよインテグレーションテストで良いと判断しました。
これはインテグレーションテストでよい理由にはならない。単体テストが望ましいのであれば、
- workflow-approver-group
- index.ts
- workflow-approver-group.ts
- workflow-approver-group.spec.ts
- workflow-approver-group.integ.ts
- remove-approver-group.ts
- remove-approver-group.spec.ts
のようなファイル構造にした上で単体テストを書くべき。
There was a problem hiding this comment.
WorkflowApproverGroupService 内部の異常系のテストを、WorkflowApproverGroup.integ.ts に切り出しました。
There was a problem hiding this comment.
すみません、まだどういう時に単体テストを書くべきなのかという基準がわかっていないのでインテグレーションテストのみ書いています。
以前 workflow-serializer.ts の単体テストを書いた時は workflow-serializer.ts 内部で依存している user-serializer を mock 化しすることで、workflow-serializer.ts 内部のオリジナルの実装部分のみをテストできるというメリットが理解できたのですが、今回のようなケースでは内部で依存している外部モジュール等がなかったので単体テストは書いていないです。
https://github.com/weseek/growi/blob/5eabb93912136c15a6c68a7cd088e21341c605a9/apps/app/src/features/approval-workflow/server/models/serializers/workflow-serializer.spec.ts
| ], | ||
| creator: { _id: new mongoose.Types.ObjectId().toString() }, | ||
| getLatestApprovedApproverGroupIndex: vi.fn(() => 1), | ||
| } as any; |
There was a problem hiding this comment.
any はやめよう
今実装の方見たらそっちにも any いっぱいあるからなくしてほしい
There was a problem hiding this comment.
これは自分も問題だと思っていて、mongose のインスタンス用の type と DB から取得してきた純粋なデータの type が混合している結果 any を使わざるを得ない状況になってしまっています。
現状は DB から取得してきた純粋なデータの type (IWorkflowHasId など) のみしかなく今回のケースのように mongoose のインスタンスを引数として取るものに対しても IWorkflow~ を使うような実装になってしまっています。
これは現状の DX も下がってしまっているので優先度を上げて (次回の見通しに入れるレベル) 後続タスク で対応しようとお思うのですがどうでしょうか？
There was a problem hiding this comment.
model にある type や interface を export するだけじゃだめなの？
apps/app/src/features/approval-workflow/server/services/workflow-approver-group.spec.ts
Show resolved
Hide resolved
| expect(caller).rejects.toThrow('Cannot edit workflows that are not in progress'); | ||
| }); | ||
|
|
||
| it('Should fail when the target approverGroup does not exist', () => { |
There was a problem hiding this comment.
expect(caller).rejects.toThrow(message) の message 部分がちゃんとテスト対象と一致する .spec.ts や .integ.ts を作るべきなので、「WorkflowApproverGroup.integ.ts に切り出すのでも良い」ではなく「切り出さないといけない」
| expect(caller).rejects.toThrow('Cannot edit workflows that are not in progress'); | ||
| }); | ||
|
|
||
| it('Should fail when the target approverGroup does not exist', () => { |
There was a problem hiding this comment.
WorkflowApproverGroupService.updateApproverGroup から呼び出されている各メソッドはプライベートメソッドになっているため (対象クラスのパブリックなメソッドから呼ばれることが前提となっている)、いずれにせよインテグレーションテストで良いと判断しました。
これはインテグレーションテストでよい理由にはならない。単体テストが望ましいのであれば、
- workflow-approver-group
- index.ts
- workflow-approver-group.ts
- workflow-approver-group.spec.ts
- workflow-approver-group.integ.ts
- remove-approver-group.ts
- remove-approver-group.spec.ts
のようなファイル構造にした上で単体テストを書くべき。
|
|
||
| // then | ||
| const createdTargetApproverGroup1 = updatedWorkflow.approverGroups[1]; | ||
| const createdTargetApproverGroup2 = updatedWorkflow.approverGroups[3]; |
There was a problem hiding this comment.
ここで assert してる対象が updateWorkflow から返ってきたドキュメントインスタンスに対してのみになっているので、
もちろんそれも必要なんだけど DB から改めて ID 指定で撮り直したドキュメントインスタンスに対しても同じ assertion をやってほしい。
…ation-of-update-workflow
Task
#131556 [approval-workflow] ワークフロー編集画面からワークフローを編集できる
└ #131908 [server] テスト実装