CNativeテストケースの期待値を修正する

[berryzplus]

PR の目的

CNativeテストケースの期待値を修正する

コメントに書いてある通り、期待値は「より大きい」で「以上」ではないので修正したい。

カテゴリ

• その他

PR の背景

LE(=less than or Equal)とすべきところをLT(=less than)としていた。

PR のメリット

PR のデメリット (トレードオフとかあれば)

• とくにありません。

PR の影響範囲

• アプリ（＝サクラエディタ）の機能に影響はありません。
• テストコードの期待値を修正します。

関連チケット

参考資料

[AppVeyorBot]

✅ Build sakura 1.0.2692 completed (commit ef22bc94df by @berryzplus)

[ds14050]

EXPECT_LE と EXPECT_LT の違いを検知できるテストケースに改める好機だと思いますがなぜやらないのでしょうか。

@berryzplus さんはローカルで失敗してはいけない EXPECT_LT が失敗することと、それを EXPECT_LE に改めることで失敗しなくなることを確認したはずです。つまり、テストが壊れていることの確認と、直ったことの確認です。

そのときのテストケースこそが、同じ失敗を繰り返さないためにこの PR に含めるべき、改善されたテストケースです。

[berryzplus]

#1230 (comment) すまんですが、何言ってるかわからんです。

単純に、CNativeWの実装改善を模索してみたときに、要求仕様（＝コメントで説明文を付けた仕様）とチェック仕様（＝testアサート）にズレがあることに気付いたから直したいだけなんですわ。

実装改善のPRに含めると、動作変えとるやんけ！ってなって寒いので。

[berryzplus]

ん？実装改善のコードを先に出せ、と言ってます？

• 試行していた改善の内容
  • CNativeWのメモリ確保部分を改良して、必要最低限のメモリ「だけ」を確保するように改善した。

必要最低限のメモリ「だけ」を確保するように修正したCNativeWでは当然テストが失敗しました。エラー時コードを出せ、ということは、失敗を正当化しろ、と言ってるようなもんで、必要最低限を超えるメモリを確保するのは「誤りだ」というようなもんだと思います。

余分なメモリを確保するのはダサいとは思いますが、誤りだとは思いません。

よって、今回変えるのは「誤って記述していたテスト条件」だけです。

[berryzplus]

ふむ。。。そもそものツッコミどころとして

「～より大きい」を判定したいのに、なんで LessThan 使ってんのよ？

ってのがありますね...orz

考え方によっちゃ、GreaterThanの左右入替ならLessThanEquealで合ってんじゃん、という見方もできるわけで、PRの趣旨がなんかおかしい気がしてきました。

[ds14050]

私のコメント( #1230 (comment) )の取り扱いはレビュアに委ねます。不問に付すなり PR 主に要求するなりご自由に。関わりを持たないのもひとつの選択です、もちろん。

[AppVeyorBot]

✅ Build sakura 1.0.2697 completed (commit 1f82c65b00 by @berryzplus)

[beru]

自分の想像になりますが ds14050 さんが言ってるのは、テストコード側で EXPECT_LE と EXPECT_LT の使い分けを間違ってしまったのであれば、新たに修正するテストコードではその両方を使いその違いを表現する事で今後の糧になる…みたいな事かもしれないです。

なお自分は被テストコードであるプロダクトコード側にバグを入れてしまいそれが見つかってしまった場合にそのタイミングでテストコードを追加しているかといったら入れてません。単に面倒なので。

話は関係無いですが doctest に移行したいです…。

[ds14050]

@beru さんに伝わらないのなら私の責任です。

EXPECT_LE と EXPECT_LT の違いが検知できるイコールが成立するようなテストケース、実装固有のエッジケースをつくような感度の高いテストケース、バッファとして最低限必要なサイズに２を足して７を足して末尾３ビットを捨てたときに内部で使用する２バイト以上の余剰バッファを確保しないようなテストケース、例えば文字列長が４の倍数マイナス１となるような wchar_t 列をテストケースとして採用する好機だということです。

[ds14050]

仕様ではなく実装に対するテストケースですから最初からそういうテストケースを書くべきだとは思いません。しかし <= と < を間違えたことを反省するなら、そして TDD の(ある一派の)手順に沿って、テストが通らないこととそれが通るようになったことを順に確認するなら、むしろやらない理由がありません。

[ds14050]

@beru「なお自分は被テストコードであるプロダクトコード側にバグを入れてしまいそれが見つかってしまった場合にそのタイミングでテストコードを追加しているかといったら入れてません。単に面倒なので。

これは非常に残念なことです。最初に網羅的なテストが書けなかった場合でも、せめて見つかったバグに対応するテストだけは手戻りを防ぐために書くべきです。それがテストを利用した開発の、レガシーコードから脱却するための、最初の最初の一歩です。

[ds14050]

もちろん、テストが面倒だからバグが放置される、という状況に陥るようでは問題があります。想定するにしてもあまりに最低な状況ですが。

[beru]

@beru さんに伝わらないのなら私の責任です。

どうなんでしょうね？単に自分の頭（理解力や想像力）が悪いだけかもしれません。会話のキャッチボールと考えれば気楽ですがそこから地獄甲子園に発展しないとも限りません。

EXPECT_LE と EXPECT_LT の違いが検知できるイコールが成立するようなテストケース、実装固有のエッジケースをつくような感度の高いテストケース、バッファとして最低限必要なサイズに２を足して７を足して末尾３ビットを捨てたときに内部で使用する２バイト以上の余剰バッファを確保しないようなテストケース、例えば文字列長が４の倍数マイナス１となるような wchar_t 列をテストケースとして採用する好機だということです。

元々 #1230 (comment) は EXPECT_LE と EXPECT_LT の違いを検知できるテストケース という話だった気がしますが上記のコメントを見るとボリュームが増えてますね。二郎でマシを頼んだらチョモランマがやってきたような感が有ります。

	EXPECT_LT(cch + 1, value.capacity());

演算子を使って分かりやすく書くとすると下記のようになるかと思います。

	CHECK(cch + 1 < value.capacity());

cch は _countof(sz) - 1 で取った値で wchar_t 文字数を表しますが、+1 しているのはNULL終端の分だと思います。

で、確か終端文字2つ分付けていたから元のテストの記述でも間違っていないんじゃないかと思ったんですがそれは自分の勘違いっぽいです。

処理の流れとしては下記のようになります。

CNativeW::CNativeW( const wchar_t* pData )
  CNativeW::SetString( const wchar_t* pszData )
    CMemory::SetRawData( const void* pData, int nDataLen )
      CMemory::AllocBuffer( int nNewDataLen )
      CMemory::_AddData( const void* pData, int nDataLen )

wchar_t のNULL終端を2文字付加しているわけではなくて、char の NULL終端を2文字追加でした。そうなると確かに元のテストの記述は間違っています。

で、EXPECT_LE と EXPECT_LT の違いを検知できるテストケース というのがなんぞやという話ですが、自分が思いついたのは下記のような記述です。

	EXPECT_LT(cch, value.capacity());
	EXPECT_LE(cch + 1, value.capacity());

うーん、これだとやっている事は同じであまり意味は無いでしょうか…。

話が変わりますが、業務でテストコードの改造をする際に、元のテストコードを書いた人がどうしてこういう期待値になるテストを書いたのかをコードから読み取るのが難しい、と感じる時があります。自然言語で解説してくれ～～!!って思ってしまいますが、書き手からするとテストケース毎にそんないちいち長文の解説書いてられるかyo! という事かもしれません。

で、コードを読んだ人がコードが分かりにくいと思った場合に書いた人の書き方がまずいのか、それとも読んだ人の頭脳に障害があるのかは脳細胞の色と同じでグレーだと思いますが、美化して玉虫色にしておきたいです。攻撃的な人は真っ赤に血で染めてきそうですが。。

あとコメントって何をやっているかを書くよりどうしてそうしたのかを書いてほしいって思うときはありますね。

[ds14050]

長くなってしまいましたが話題は３つです。水平線で区切っています。

---

beru さんにはどれだけ低く見積もっても人並みの、普通に人並み以上の理解力とコーディング能力があると思っていますから、伝えられない方に問題があります。

元々 #1230 (comment) は EXPECT_LE と EXPECT_LT の違いを検知できるテストケース という話だった気がしますが上記のコメントを見るとボリュームが増えてますね。

すべて同じことを別の表現で言い直しただけです。すべてがひとつの対象を理解するための手がかりです。しかし並記されたものの関係は明らかではありませんでしたね。

本流からは逸れてしまいますが、beru さんのアプローチでテスト条件を理解する際のヒントです。CMemory::capacity 関数は実際に確保したサイズから -2 を補正した値を返します。CNativeW::capacity 関数はその補正されたバイト数を wchar_t 単位に変換します。

cch は _countof(sz) - 1 で取った値で wchar_t 文字数を表しますが、+1 しているのはNULL終端の分だと思います。

CNativeW はヌル終端を明示的には確保せず CMemory に任せ、そのために CNativeW::capacity 関数の戻り値はヌル終端を反映していないと思います。先ほど書いたように CMemory::capacity が -2 (バイト)を補正しています。

そもそもがヌル文字を包含しうる文字列型ですから、ヌル終端を加えたら内容の改竄と同じことです。内部的にヌル終端があったとしても、それを見せるのは誤りですから見せません。

---

このように書きました。

そして TDD の(ある一派の)手順に沿って、テストが通らないこととそれが通るようになったことを順に確認するなら、むしろやらない理由がありません。

これは何も特別な宗派が求める特別な手続きというわけではありません。足元を一歩一歩確かめながら進める慎重な手続きというだけです。今回なら以下のような手順を踏みます。

1. テスト条件(EXPECT_LT)が誤っている気がしたのでそれを明らかにするようなテストケースを考える。>CNativeテストケースの期待値を修正する #1230 (comment)
2. 本当に誤っていたならテストが失敗するのでどこを直すのか考える。テスト条件？ テストケース？ アプリケーション実装？ 仕様ドキュメント？

テストは本当に壊れていたのかもしれませんし、壊れていなかったかもしれません。実は別のところが壊れていたのかもしれません。思い込みを排除するために実際の挙動を判断材料にします。テスト結果によって壊れていることを確認し、テスト結果によって直ったことを確認します。

バグ報告に再現手順を含めることが求められるのも同じでしょう。バグの存在を確認し、バグが取れたことを確認するためです。『Joel on Software』のジョエルさんのコラムで読みましたが、正確を期するならバグが取れたことを確認できるのはバグ報告者だけです。それをどちらもあてずっぽうでやる者がいますが。

---

業務でテストコードは改造をする際に、テストコードを書いた人がどうしてこういう期待値になるテストを書いたのかをコードから読み取るのが難しい、と感じる時があります。自然言語で解説してくれ～～!!って思ってしまいますが、書き手からするとテストケース毎にそんないちいち長文の解説書いてられるかyo! という事かもしれません。

テストは半分コード、半分ドキュメントみたいな雰囲気がありますね。自然言語に似せて書くフレームワークがあったり、関数名を英語以外のネイティブランゲージにしてみたり。文字列リテラルなども説明を埋め込む適所になりえます。アプリケーションコード以上に説明の努力が求められていると感じます。

書いた本人にとってコードの前提となった知識、思考の流れは当たり前のものとして存在していますから、それを抜きにした他者に生じる疑問を予め想定しにくいところがあります。相手がいるなら「ここがよくわからない」とまず言ってみては？ それに対する反応は様々でしょうけれど。

「解らないはずがない」「お前は馬鹿だ」「理解する努力をしろ」「解りにくかったかもね。でも解るでしょ」「じゃあちょっと説明を足すよ」

あとコメントって何をやっているかを書くよりどうしてそうしたのかを書いてほしいって思うときはありますね。

その前半部分を「ソースコードの翻訳コメントは駄目コメントの典型」という表現でここに書いたことがあります。指摘がないと案外わかっていてもやってしまうかもしれません。わかっているので応じて改めるのは簡単ですが。

[berryzplus]

条件をGTに変えると、テストコードの見た目的にキモいことに気付きました。
GT使うと期待値が左側に来ないから、並べて書いたときに揃わない・・・。

う～む、会話のやりとりで目指す場所がわからんです。

いまある単体テストコードって、初心者向けのすっごく簡単なやつです。

なんだかエラく高次元の高尚な話題をしている気がするんですが、ここのテストってまだ全然そんなレベルに達してない気がしていて、何がしたいかわからん感じです。

大八車にF1のエンジン載せようとしてる感じ？
載るのかもしれませんが、きっと意味無いっす。

このPRの意図は、いまあるやつってなんか違くね？こうしたらいいと思うんだけど、どう？です。

そこを無視してスーパー理想論に走ってるような印象を受けました。

[AppVeyorBot]

✅ Build sakura 1.0.2701 completed (commit e520093944 by @berryzplus)

[beru]

@ds14050 さん

すべて同じことを別の表現で言い直しただけです。すべてがひとつの対象を理解するための手がかりです。しかし並記されたものの関係は明らかではありませんでしたね。

#1230 (comment) の別の表現が #1230 (comment) という事のようなので圧縮率高いなぁと思います。結構当たり前のようなことでも冗長に書いた方が誤解等が生じにくい気もしますが、少ない情報からでも補間して把握できない方にも問題があるのも確かなのでやり取りは難しいですね。。

ひとつの対象 というのが何なのかは具体的に明記されていないのでこちらは推測するしか無いですが、CMemory::AllocBuffer のメモリ確保の仕方、及び CNativeW::capacity の戻り値の意味に応じて境界条件の確認をした方が良いんじゃとかそんな辺りなんでしょうか？

cch は _countof(sz) - 1 で取った値で wchar_t 文字数を表しますが、+1 しているのはNULL終端の分だと思います。

CNativeW はヌル終端を明示的には確保せず CMemory に任せ、そのために CNativeW::capacity 関数の戻り値はヌル終端を反映していないと思います。先ほど書いたように CMemory::capacity が -2 (バイト)を補正しています。

そもそもがヌル文字を包含しうる文字列型ですから、ヌル終端を加えたら内容の改竄と同じことです。内部的にヌル終端があったとしても、それを見せるのは誤りですから見せません。

CNativeW::capacity がNULL終端分を含まないっていうのは自分はきちんと認識が出来てませんでした。capacity っていう名前からSTLのコンテナのようにメモリ確保している最大要素数が返ってくるかと思い込んでいたんですが、末尾のNULL終端を除外して何文字分保持できるかという数値なんですね。トリッキーだなぁ…。

というかもはや CNativeA ってあまり使われてないし CNativeW を CString に改名して CMemory 継承しない作りにした方が実装が明解になるんじゃ？って思う事がよくあります。CNativeW っていう名前の Native ってそもそも一体何だろう…。

これは何も特別な宗派が求める特別な手続きというわけではありません。足元を一歩一歩確かめながら進める慎重な手続きというだけです。今回なら以下のような手順を踏みます。

1. テスト条件(EXPECT_LT)が誤っている気がしたのでそれを明らかにするようなテストケースを考える。>#1230 (comment)
2. 本当に誤っていたならテストが失敗するのでどこを直すのか考える。テスト条件？ テストケース？ アプリケーション実装？ 仕様ドキュメント？

テストは本当に壊れていたのかもしれませんし、壊れていなかったかもしれません。実は別のところが壊れていたのかもしれません。思い込みを排除するために実際の挙動を判断材料にします。テスト結果によって壊れていることを確認し、テスト結果によって直ったことを確認します。

バグ報告に再現手順を含めることが求められるのも同じでしょう。バグの存在を確認し、バグが取れたことを確認するためです。『Joel on Software』のジョエルさんのコラムで読みましたが、正確を期するならバグが取れたことを確認できるのはバグ報告者だけです。それをどちらもあてずっぽうでやる者がいますが。

まぁ色々とゆっくり慎重に何をやっているかを明確にしながら進めていった方が良いのかもしれないですね。急がば回れというか…。

バグ報告は再現手順がちゃんと付いているとありがたいですね。不具合を見つけても報告しない人が殆どなので報告してくれるだけでも感謝ですが…。

テストは半分コード、半分ドキュメントみたいな雰囲気がありますね。自然言語に似せて書くフレームワークがあったり、関数名を英語以外のネイティブランゲージにしてみたり。文字列リテラルなども説明を埋め込む適所になりえます。アプリケーションコード以上に説明の努力が求められていると感じます。

書く側になると読む側の立場になれないってのはまぁ当然かもしれないですね。説明を入れるのにはその分コストが掛かるし、説明が豊富にあっても設計の問題はまた別物なので。

書いた本人にとってコードの前提となった知識、思考の流れは当たり前のものとして存在していますから、それを抜きにした他者に生じる疑問を予め想定しにくいところがあります。相手がいるなら「ここがよくわからない」とまず言ってみては？ それに対する反応は様々でしょうけれど。

「解らないはずがない」「お前は馬鹿だ」「理解する努力をしろ」「解りにくかったかもね。でも解るでしょ」「じゃあちょっと説明を足すよ」

なお書いた相手は近くにいないし書かれたのも過去の事だと思うのであえて細かく聞くのも気が引けてしまう状況だったりします。自分がもし相手で説明を求められたとしたら「ゆっくりしていってね！！！」と返しそう…。

その前半部分を「ソースコードの翻訳コメントは駄目コメントの典型」という表現でここに書いたことがあります。指摘がないと案外わかっていてもやってしまうかもしれません。わかっているので応じて改めるのは簡単ですが。

確かに自分でもよくやってしまっていそう…。

[beru]

問題無いと思います。

GetStringLength の戻り値に対しては EQ で、
capacity の戻り値に対しては LE を使って判定する事で
関係性が見えます。

コメントも修正されてますね。

[beru]

う～む、会話のやりとりで目指す場所がわからんです。

いまある単体テストコードって、初心者向けのすっごく簡単なやつです。

ここの単体テストコードの記述自体は簡単なものなのかもしれないですけど、CNativeW : CNative : CMemory っていう継承構造の実装でコード読まないと戻り値が何を意味するのかとか分からないところには難しさを感じます。

なんだかエラく高次元の高尚な話題をしている気がするんですが、ここのテストってまだ全然そんなレベルに達してない気がしていて、何がしたいかわからん感じです。

大八車にF1のエンジン載せようとしてる感じ？
載るのかもしれませんが、きっと意味無いっす。

@ds14050 さんは原理主義的に原則というか基本を提示したのかもしれないですね。ただなんか言い方が遠回しというか婉曲的に感じますね。コードレビューの場合は具体的に行指定でこういうコードにしたら良いというコード例を提示するやり方も合わせてやった方が確実な気はします。

このPRの意図は、いまあるやつってなんか違くね？こうしたらいいと思うんだけど、どう？です。

そこを無視してスーパー理想論に走ってるような印象を受けました。

なんか違くね？ だと曖昧すぎてまずくないっすか？明確にした方がいいような…。でもまぁ会話で省略を許さない厳密を求める人間になってしまうと頭の回転が悪い病気というか、会話してツッコミばっかりが多い嫌な人間になりそうです。:sob:

[beru]

EXPECT_LE と EXPECT_LT の違いが検知できるイコールが成立するようなテストケース、実装固有のエッジケースをつくような感度の高いテストケース、バッファとして最低限必要なサイズに２を足して７を足して末尾３ビットを捨てたときに内部で使用する２バイト以上の余剰バッファを確保しないようなテストケース、例えば文字列長が４の倍数マイナス１となるような wchar_t 列をテストケースとして採用する好機だということです。

メモリ確保の内部仕様を前提にはしないとしても、境界の関係でたまたまテストが通る事もあるので、固定文字数の文字列リテラルではなくてループで色々な文字数の場合の試験を追加した方が良いかもしれないですね。

しかしまぁそもそも @ds14050 さんが示唆するコメント付けるより自分でコード書いてPR出した方が良いんじゃ？とか思ってしまいますが、それはそれで働いたら負けとかいう事かもしれません。

[ds14050]

しかしまぁそもそも @ds14050 さんが示唆するコメント付けるより自分でコード書いてPR出した方が良いんじゃ？とか思ってしまいますが、それはそれで働いたら負けとかいう事かもしれません。

理由はいろいろ挙げられます。怠け者だから、理解されないだろうから、「お前は馬鹿だ」と言うことが目的だから。

[berryzplus]

レビューありがとうございます。
マージしちゃいます。

[berryzplus]

有益なコメントを色々スルーしてしまってるのはすんません。

たぶん、あとで振り返って引用することになるのかな。

その遥か先の未来に、「だからあんとき言うたやんけ！」と謎ギレするのがこの会話の主たる目的なのなのだろう、と理解してます。

着実に進んでいくためならば、とりあえずバカで構わん、というのはぼく個人の指針です。

[ds14050]

目眩ましにもならない空疎な言葉。学ばないよね。そしてこの面の皮>「動作確認はしていませんが。」

暗澹たる思いとともにもう何度目かの絶望を味わっています。なぜ本人が「(バカで)構わん」と言えるのか不思議ですが、それを受け入れるプロジェクトメンバーの度量はたしかに私の想像を超えています。(一人を除いて)健闘を！