Term Balance Object#40
Conversation
The following actions uses node12 which is deprecated and will be forced to run on node16: actions/checkout@v2, actions/setup-java@v2. For more info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
| - uses: actions/checkout@v4 | ||
| - name: Set up JDK | ||
| uses: actions/setup-java@v2 | ||
| uses: actions/setup-java@v4 |
There was a problem hiding this comment.
直近のactionsで下記のメッセージが出てた
The following actions uses node12 which is deprecated and will be forced to run on node16: actions/checkout@v2, actions/setup-java@v2. For more info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
| String json = resource("term_event.json"); | ||
| Event e = gson.fromJson(json, Event.class); | ||
|
|
||
| assertEquals(Term.class, e.getData().getClass()); |
There was a problem hiding this comment.
PayjpObjectDeserializerにTermへの変換を追加しないとPayjpObjectになってこれが落ちる
|
|
||
| public class BalanceTest extends BasePayjpTest { | ||
| @Test | ||
| public void testDeserialize() throws PayjpException, IOException { |
There was a problem hiding this comment.
テストで確かめているのは
- レスポンスJSONが指定のオブジェクトに変換される
- 子供のオブジェクトがあればそれも想定の型に変換される
- リクエストURLが /v1/balances/:id , /v1/balances である
あたり
| assertEquals("tm_b92b879e60f62b532d6756ae12aa", term.getId()); | ||
| assertEquals(Long.valueOf("1438354812"), term.getCreated()); | ||
| assertEquals((Integer)987, term.getChargeCount()); |
There was a problem hiding this comment.
大した数ではないので Term オブジェクトの中身全部チェックしてもいいかと思います。
getter のテストということで。逆にここで動いていれば他の箇所は全チェックはなくてもよい。
There was a problem hiding this comment.
assert足しました。BalanceTestも。
ついでに目についたとこリファクタと、listのリクエスト先URLがおかしいバグが見つかったので修正しました
| Term term = terms.get(0); | ||
| String id = term.getId(); | ||
| assertEquals(id, "term1"); |
There was a problem hiding this comment.
List のテストなので中身２つ作ってテストしてもいいとは思いました。
{
"count": 2,
"data": [
{"id": "term1"},
{"id": "term2"},
]
}| assertEquals("ba_b92b879e60f62b532d6756ae56af", balance.getId()); | ||
| assertEquals(Long.valueOf("1438354824"), balance.getCreated()); | ||
| assertEquals(BigInteger.valueOf(Long.valueOf("12300000000")), balance.getNet()); | ||
| StatementCollection statements = balance.getStatements(); | ||
| assertEquals("st_178fd25dc7ab7b75906f1c3c4b0e6", statements.getData().get(0).getId()); | ||
| assertEquals("st_b4a569b0122a7d08b358f198cf263", statements.getData().get(1).getId()); |
There was a problem hiding this comment.
全チェックしてもいいのではと思いました。
テストしない理由があれば聞いておきたいです。
bank_info とか null ありえますよね。そういうテストのほうが大事な気がしました。
| public BankInfo getBankInfo() { | ||
| return bankInfo; | ||
| } |
There was a problem hiding this comment.
bank_info は null があり得るので、ぬるぽ出ませんかね？
balance.getBankInfo().getBankCode()他にも該当するコードはありそう。
There was a problem hiding this comment.
GSONでのシリアライズ時点では出ないです。一応ローカルのbalance.jsonに "bank_info": null 書いて確かめましたがbalance.getBankInfo()でもnullが返ってくるだけ。
Javaのプリミティブ型以外は全部nullがあり得るのでその先のnull考慮は使用者の責任かと。
以降のnullの話も同じくです
| objectMap.put("term", Term.class); | ||
| objectMap.put("statement", Statement.class); | ||
| objectMap.put("balance", Balance.class); |
There was a problem hiding this comment.
tab と space 気になったけどどっちに揃えるのがいいんでしょう。
There was a problem hiding this comment.
4スペースが多そう
- oracle https://www.oracle.com/docs/tech/java/codeconventions.pdf
- AndroidソースのJava https://source.android.com/docs/setup/contribute/code-style?hl=ja
現状いろんなとこで入り混じってるので揃えるなら別PRにしてlinter入れたい
| public String getDueDate() { | ||
| return dueDate; | ||
| } |
There was a problem hiding this comment.
ここも null ありえそうですが、String のメソッド使えないくらいなので影響はそれほどなんですかね。
これは質問の割合のほうが大きいです。
| public void testList() throws PayjpException { | ||
| Map<String, Object> listParams = new HashMap<String, Object>(); | ||
| listParams.put("limit", 1); | ||
| stubNetwork(BalanceCollection.class, "{\"count\":1,\"data\":[{\"id\":\"balance1\"}]}"); | ||
| List<Balance> balances = Balance.all(listParams).getData(); | ||
| Balance balance = balances.get(0); | ||
| String id = balance.getId(); | ||
| assertEquals(id, "balance1"); | ||
| } |
| "bank_info": { | ||
| "bank_code": "0000", | ||
| "bank_branch_code": "123", | ||
| "bank_account_type": "普通", | ||
| "bank_account_number": "1234567", | ||
| "bank_account_holder_name": "ペイ タロウ", | ||
| "bank_account_status": "pending" | ||
| } |
There was a problem hiding this comment.
上のほうでコメントした null 関連のやつ、ここが null でテスト通るなら全然スルーで良い認識。
There was a problem hiding this comment.
めちゃ細かくて申し訳ない。
L33 の import jp.pay.model.Event; 使ってなさそうなら削除したいです。
Co-authored-by: Nozomi Hosaka <nozomi.hosaka@binc.jp>
| public BankInfo getBankInfo() { | ||
| return bankInfo; | ||
| } |
There was a problem hiding this comment.
GSONでのシリアライズ時点では出ないです。一応ローカルのbalance.jsonに "bank_info": null 書いて確かめましたがbalance.getBankInfo()でもnullが返ってくるだけ。
Javaのプリミティブ型以外は全部nullがあり得るのでその先のnull考慮は使用者の責任かと。
以降のnullの話も同じくです
| objectMap.put("term", Term.class); | ||
| objectMap.put("statement", Statement.class); | ||
| objectMap.put("balance", Balance.class); |
There was a problem hiding this comment.
4スペースが多そう
- oracle https://www.oracle.com/docs/tech/java/codeconventions.pdf
- AndroidソースのJava https://source.android.com/docs/setup/contribute/code-style?hl=ja
現状いろんなとこで入り混じってるので揃えるなら別PRにしてlinter入れたい
| assertEquals("tm_b92b879e60f62b532d6756ae12aa", term.getId()); | ||
| assertEquals(Long.valueOf("1438354812"), term.getCreated()); | ||
| assertEquals((Integer)987, term.getChargeCount()); |
There was a problem hiding this comment.
assert足しました。BalanceTestも。
ついでに目についたとこリファクタと、listのリクエスト先URLがおかしいバグが見つかったので修正しました
| Term term = terms.get(0); | ||
| String id = term.getId(); | ||
| assertEquals(id, "term1"); |
|
|
||
| public BigInteger getNet() { | ||
| return net; | ||
| } |
There was a problem hiding this comment.
title の getter がなさそう？
他には updated の getter がなさそうですが理由はあるように見えるので聞いておきたいです。
2 participants
Uh oh!
There was an error while loading. Please reload this page.